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: Permit message, dataTree value incorrectly using default ERC20 decimals for non-ERC20 token values #28142

Merged
merged 6 commits into from
Nov 5, 2024

Conversation

digiwand
Copy link
Contributor

@digiwand digiwand commented Oct 29, 2024

Description

Removes bug where fetchErc20Decimals was used in ui/pages/confirmations/components/confirm/info/typed-sign/typed-sign.tsx info component and passed to the message dataTree to be used. fetchErc20Decimals was incorrectly used as it cannot be assumed that the token contract is an ERC20 standard. Fixed by calling useGetTokenStandardAndDetails instead which will use the default 18 decimals digit if the standard is an ERC20 token with no decimals found in the details.

Open in GitHub Codespaces

Related issues

Fixes: #28118

Manual testing steps

  1. Go to test-dapp
  2. Test Permit button
  3. Observe both the simulation and message value display "3,000" (contract is not an ERC20 standard so it does not apply default 18 decimals)

Screenshots/Recordings

Before

After

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.

@digiwand digiwand requested a review from a team as a code owner October 29, 2024 10:12
@digiwand digiwand added the team-confirmations Push issues to confirmations team label Oct 29, 2024
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.

@digiwand digiwand changed the title fix: Permit dataTree should not enforce ERC20 18 digit decimals if to… fix: Permit message, dataTree value incorrectly using default ERC20 decimals for non-ERC20 token values Oct 29, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [0239fa5]
Page Load Metrics (1960 ± 86 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint23824881867412198
domContentLoaded17262409192917986
load17812426196017986
domInteractive18107562311
backgroundConnect8102322512
firstReactRender52208953115
getState581342612
initialActions01000
loadScripts12901951145416680
setupStore1193272512
uiStartup198126812188222106
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: -488 Bytes (-0.01%)
  • common: 0 Bytes (0.00%)

@metamaskbot
Copy link
Collaborator

Builds ready [876ae40]
Page Load Metrics (2215 ± 135 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint25428242025646310
domContentLoaded175127792173289139
load177528262215282135
domInteractive1992502311
backgroundConnect9112403517
firstReactRender792251233015
getState685232512
initialActions01000
loadScripts127122441604251121
setupStore12101332512
uiStartup197434032483344165
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: -488 Bytes (-0.01%)
  • common: 0 Bytes (0.00%)

matthewwalsh0
matthewwalsh0 previously approved these changes Nov 1, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [0207051]
Page Load Metrics (1844 ± 100 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint30325261771394189
domContentLoaded16042454181319795
load164725291844207100
domInteractive207541157
backgroundConnect894312110
firstReactRender48114832512
getState472272311
initialActions01000
loadScripts11571805134015575
setupStore116332209
uiStartup181527442046226109
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: -488 Bytes (-0.01%)
  • common: 0 Bytes (0.00%)

@digiwand digiwand added this pull request to the merge queue Nov 5, 2024
Merged via the queue into develop with commit 9fb69f4 Nov 5, 2024
76 checks passed
@digiwand digiwand deleted the fix-permit-default-erc20-decimal-values branch November 5, 2024 10:59
@github-actions github-actions bot locked and limited conversation to collaborators Nov 5, 2024
@metamaskbot metamaskbot added the release-12.8.0 Issue or pull request that will be included in release 12.8.0 label Nov 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.8.0 Issue or pull request that will be included in release 12.8.0 team-confirmations Push issues to confirmations team
Projects
None yet
4 participants