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: token decimals fetched from the chain #7540

Merged
merged 7 commits into from
Oct 20, 2023

Conversation

tommasini
Copy link
Contributor

@tommasini tommasini commented Oct 18, 2023

Description

The problem was that the user was able to insert wrong the token decimals values, and with that would get the wrong tokens amounts in their wallet.

  • Added a new state to lock the text inputs of symbol and decimals when the symbol
  • Decimals and names are fetchable from the chain
  • Reset all the states when the address text input changed when an address does not have 42 chars
  • Added styling to address the value of symbols and decimals text input when fetched from the chain

It was also added to the wallet_watchAsset JSON rpc method

  • overrides token decimals and symbol if they aare fetched from the chain

Note for review

  • On RPCMethodMiddleware file wallet_watchAsset method
  • On the 207 line validateCustomTokenAddress of AddCustomToken

Manual testing steps

Scenario 1:
_1. Step1: Go to import token on wallet view
_2. Step2: change tab to custom token
_3. Step3: add 0x2260FAC5E5542a773Aa44fBCfeDf7C193bc2C599 (WBTC) as token address
_4. Step4: It should block all the symbol and decimals field

Scenario 2
Step1: Go to in app browser
Step2: Go to coingecko website
Step3: Browse WBTC
Step4: look for the MM icon
Step5: Add it via coin gecko
Step6: It should work as expected

Screenshots/Recordings

E2E build

https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/1d5eb4d0-faff-456b-b66b-6e55718f4cdf

Related issues

_Fixes #

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've clearly explained:
    • What problem this PR is solving.
    • How this problem was solved.
    • How reviewers can test my changes.
  • I’ve indicated what issue this PR is linked to: Fixes #???
  • I’ve included tests if applicable.
  • I’ve documented any added code.
  • I’ve applied the right labels on the PR (see labeling guidelines).
  • 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.

… the symbol, decimals and name are fetchable from the chain, also reseted all the state when the address text input change when an address does not have 42 chars, added styling to address the value of symbols and decimals text input when fetched from the chain
@tommasini tommasini added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) team-mobile-client labels Oct 18, 2023
@tommasini tommasini requested a review from a team as a code owner October 18, 2023 21:28
@github-actions
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.

@gauthierpetetin gauthierpetetin added in-progress and removed needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Oct 18, 2023
@gauthierpetetin gauthierpetetin added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) and removed in-progress labels Oct 18, 2023
…lues from the chain on wallet_watchAsset jsonRPC method
@legobeat
Copy link
Contributor

Can test(s) be added for this invariant?

Copy link
Contributor

@legobeat legobeat left a comment

Choose a reason for hiding this comment

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

app/components/UI/AddCustomToken/index.js Show resolved Hide resolved
app/components/UI/AddCustomToken/index.js Outdated Show resolved Hide resolved
app/components/UI/AddCustomToken/index.js Outdated Show resolved Hide resolved
@Cal-L Cal-L added the needs-qa Any New Features that needs a full manual QA prior to being added to a release. label Oct 18, 2023
@codecov-commenter
Copy link

codecov-commenter commented Oct 19, 2023

Codecov Report

Attention: 19 lines in your changes are missing coverage. Please review.

Comparison is base (bf25c3e) 34.61% compared to head (6e627fc) 34.69%.
Report is 12 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7540      +/-   ##
==========================================
+ Coverage   34.61%   34.69%   +0.07%     
==========================================
  Files        1019     1020       +1     
  Lines       27193    27249      +56     
  Branches     2218     2228      +10     
==========================================
+ Hits         9413     9453      +40     
- Misses      17289    17301      +12     
- Partials      491      495       +4     
Files Coverage Δ
app/constants/error.ts 100.00% <100.00%> (ø)
app/core/RPCMethods/index.js 100.00% <ø> (ø)
app/core/RPCMethods/RPCMethodMiddleware.ts 46.06% <0.00%> (+1.91%) ⬆️
app/core/RPCMethods/wallet_watchAsset.ts 82.60% <82.60%> (ø)
app/components/UI/AddCustomToken/index.js 30.90% <22.22%> (-0.58%) ⬇️

... and 11 files with indirect coverage changes

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

@tommasini
Copy link
Contributor Author

@legobeat Addressed the test failure, the unit test would be a blocker for you? I'm having a hard time with this unit test and I feel the logic that will be tested is the override logic due to mocking the other results. Let me know your thoughts <3

@Cal-L Addressed all your reviews thanks <3

@cortisiko cortisiko removed the needs-qa Any New Features that needs a full manual QA prior to being added to a release. label Oct 19, 2023
@Cal-L Cal-L added needs-qa Any New Features that needs a full manual QA prior to being added to a release. and removed needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Oct 20, 2023
@sonarcloud
Copy link

sonarcloud bot commented Oct 20, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

60.7% 60.7% Coverage
0.0% 0.0% Duplication

@legobeat legobeat dismissed their stale review October 20, 2023 01:05

Test has been added

@Andepande Andepande added QA Passed A successful QA run through has been done and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Oct 20, 2023
Copy link
Member

@Andepande Andepande left a comment

Choose a reason for hiding this comment

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

QA Passed

@tommasini tommasini merged commit 8fd310c into main Oct 20, 2023
26 of 29 checks passed
@tommasini tommasini deleted the fix/1307-check-decimals-on-chain branch October 20, 2023 13:44
@github-actions github-actions bot locked and limited conversation to collaborators Oct 20, 2023
@metamaskbot metamaskbot added the release-7.11.0 Issue or pull request that will be included in release 7.11.0 label Oct 20, 2023
@tommasini tommasini added release-7.10.0 and removed release-7.11.0 Issue or pull request that will be included in release 7.11.0 labels Oct 23, 2023
@gauthierpetetin gauthierpetetin added team-mobile-platform team-mobile-ux DEPRECATED: please use "team-wallet-ux" label instead labels Feb 2, 2024
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.10.0 team-mobile-platform team-mobile-ux DEPRECATED: please use "team-wallet-ux" label instead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants