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

chore: fix issue 27079 incorrect 0 balance #27083

Merged
merged 3 commits into from
Sep 12, 2024
Merged

chore: fix issue 27079 incorrect 0 balance #27083

merged 3 commits into from
Sep 12, 2024

Conversation

ejwessel
Copy link
Contributor

@ejwessel ejwessel commented Sep 11, 2024

Description

If a token has decimals as 0 it's truthly evaluated to false. This PR checks that the decimals >= 0 to prevent the balance from being evaluated as undefined and ultimately showing an improper balance.

Open in GitHub Codespaces

Related issues

Fixes:
#27079

Manual testing steps

  1. Select a token whose decimal() is 0 (ETH:0xaec2e87e0a235266d9c5adc9deb4b2e29b54d009)
  2. Click Send and fill in the acceptance address
  3. On the sending page, when filling in the sending amount verify that a balance is shown.

Screenshots/Recordings

Before

Screenshot 2024-09-11 at 16 03 16

After

Screenshot 2024-09-11 at 16 02 39

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.

@ejwessel ejwessel requested a review from a team as a code owner September 11, 2024 22:59
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.

@metamaskbot
Copy link
Collaborator

Builds ready [c1018c2]
Page Load Metrics (1837 ± 125 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint158324041846259124
domContentLoaded157623901819260125
load158324061837260125
domInteractive23171453215
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 3 Bytes (0.00%)

@@ -2679,7 +2679,7 @@ export function updateSendAsset(

if (details.standard === TokenStandard.ERC20) {
asset.balance =
details.balance && details.decimals
details.balance && details.decimals >= 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
details.balance && details.decimals >= 0
details.balance && typeof details.decimals === 'number'

I guess we're actually interested in checking if it's properly defined as a number at all? This avoid typecasting as part of comparison.

Copy link
Contributor

@legobeat legobeat Sep 12, 2024

Choose a reason for hiding this comment

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

Should it actually handle and forward 0 balance as well, to clearly distinguish between the cases balance is known to be 0 and balance undefined or couldn't be determined?

Suggested change
details.balance && details.decimals >= 0
typeof details.balance === 'string' && typeof details.decimals === 'number'

@legobeat legobeat requested a review from a team September 12, 2024 16:54
ejwessel and others added 3 commits September 12, 2024 10:51

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Copy link

@metamaskbot
Copy link
Collaborator

Builds ready [91a937b]
Page Load Metrics (1765 ± 97 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint21623031622428205
domContentLoaded15002296174420398
load15102302176520397
domInteractive1399352110
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 17 Bytes (0.00%)

Copy link

codecov bot commented Sep 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.03%. Comparing base (c7f4f43) to head (91a937b).

Additional details and impacted files
@@           Coverage Diff            @@
##           develop   #27083   +/-   ##
========================================
  Coverage    70.03%   70.03%           
========================================
  Files         1433     1433           
  Lines        49879    49879           
  Branches     13971    13971           
========================================
  Hits         34928    34928           
  Misses       14951    14951           

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

@ejwessel ejwessel merged commit 0ba1114 into develop Sep 12, 2024
78 checks passed
@ejwessel ejwessel deleted the fix-issue-27079 branch September 12, 2024 18:29
@github-actions github-actions bot locked and limited conversation to collaborators Sep 12, 2024
@metamaskbot metamaskbot added the release-12.6.0 Issue or pull request that will be included in release 12.6.0 label Sep 12, 2024
@metamaskbot metamaskbot added release-12.5.0 Issue or pull request that will be included in release 12.5.0 and removed release-12.6.0 Issue or pull request that will be included in release 12.6.0 labels Sep 29, 2024
@metamaskbot
Copy link
Collaborator

Missing release label release-12.5.0 on PR. Adding release label release-12.5.0 on PR and removing other release labels(release-12.6.0), as PR was added to branch 12.5.0 when release was cut.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.5.0 Issue or pull request that will be included in release 12.5.0 team-bridge
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants