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

[Bug]: Fix Permit token decimal amounts #27243

Closed
digiwand opened this issue Sep 18, 2024 · 2 comments · Fixed by #27328
Closed

[Bug]: Fix Permit token decimal amounts #27243

digiwand opened this issue Sep 18, 2024 · 2 comments · Fixed by #27328
Assignees
Labels
regression-develop Regression bug that was found on development branch, but not yet present in production release-12.6.0 Issue or pull request that will be included in release 12.6.0 Sev2-normal Normal severity; minor loss of service or inconvenience. team-confirmations Push issues to confirmations team type-bug

Comments

@digiwand
Copy link
Contributor

digiwand commented Sep 18, 2024

Describe the bug

If no contract was found within the Permit DataTree component, we should use the verifiedContract tokenDecimals. Passing the verifiedContract's tokenDecimals, when no decimals are found for this contract, will apply the default 18 token decimals. This is missing. Notice in the screenshot that Spending Cap displays "< 0.000001", while Message > Value displays "3,000". Value should also display "< 0.000001".

Details of applying default 18 token decimals to Redesigned Permit

When we adopted fetchErc20Decimals for Permit Simulation, we enforced a default of 18 token decimals to be used when no token decimals were found in the details. fetchErc20Decimals is the same method used in ui/pages/confirmations/components/simulation-details/useBalanceChanges.test.ts which is why we adopted it.

Previously, clicking on test-dapp > Permit would display 3,000. Now, it displays 0 < 0.000001.

@dbrans:

18 is the common convention for ERC20 tokens, largely due to OpenZeppelin's widely-used implementation. Vitalik proposed making it a required standard, though it didn’t gain momentum. EIP-1046 also suggests 18 as the default if decimals() isn’t implemented.

That said, some platforms, like Etherscan and certain wallets, default to zero when decimals() isn't defined, so there's a lot of inconsistency out there.

Expected behavior

No response

Screenshots/Recordings

Notice that Simulation > Spending Cap value does not match Message > Value

CleanShot 2024-09-18 at 17 53 09@2x

Steps to reproduce

  1. Go to test-dapp
  2. Click Permit button
  3. Observe Spending Cap and Message > Value

Error messages or log output

No response

Detection stage

On the development branch

Version

12.2.2

Build type

None

Browser

Chrome

Operating system

MacOS

Hardware wallet

No response

Additional context

No response

Severity

No response

@metamaskbot metamaskbot added the regression-develop Regression bug that was found on development branch, but not yet present in production label Sep 18, 2024
@digiwand digiwand changed the title [Bug]: Permit Simulation token value should match message value [Bug]: Investigate and apply Permit token decimal value to Spending Cap and Message values Sep 18, 2024
@digiwand digiwand added the team-confirmations Push issues to confirmations team label Sep 18, 2024
@gauthierpetetin gauthierpetetin added the Sev2-normal Normal severity; minor loss of service or inconvenience. label Sep 19, 2024
@bschorchit
Copy link

As discussed in the DSU, we can adopt the 18 decimals as the fallback and add metrics to capture when we fail to fetch decimals.
For metrics, we can re-use this event that we created for simulations:

event: Incomplete Asset Displayed

properties:
token_decimals_available: false
asset_address: < token address >
asset_type: erc20
chain_id: < chain id >
location: confirmation

@digiwand digiwand changed the title [Bug]: Investigate and apply Permit token decimal value to Spending Cap and Message values [Bug]: Investigate and apply token decimals to all Permit values Sep 23, 2024
@digiwand digiwand changed the title [Bug]: Investigate and apply token decimals to all Permit values [Bug]: Apply verifiedContract's default token decimals to Permit and add "Incomplete Asset Displayed" metric Sep 23, 2024
@digiwand digiwand changed the title [Bug]: Apply verifiedContract's default token decimals to Permit and add "Incomplete Asset Displayed" metric [Bug]: Apply verifiedContract's default token decimals to Permit values and add "Incomplete Asset Displayed" metric Sep 23, 2024
@digiwand digiwand changed the title [Bug]: Apply verifiedContract's default token decimals to Permit values and add "Incomplete Asset Displayed" metric [Bug]: Apply verifiedContract's token decimals to Permit values when no dataTree contract is found and add "Incomplete Asset Displayed" metric Sep 23, 2024
@digiwand digiwand changed the title [Bug]: Apply verifiedContract's token decimals to Permit values when no dataTree contract is found and add "Incomplete Asset Displayed" metric [Bug]: Fix Permit token decimal amounts and add "Incomplete Asset Displayed" metric Sep 23, 2024
@digiwand digiwand changed the title [Bug]: Fix Permit token decimal amounts and add "Incomplete Asset Displayed" metric [Bug]: Fix Permit token decimal amounts Sep 23, 2024
@digiwand
Copy link
Contributor Author

thank you @bschorchit! I created a separate ticket for this one: #27333

@digiwand digiwand self-assigned this Sep 23, 2024
@metamaskbot metamaskbot added the release-12.6.0 Issue or pull request that will be included in release 12.6.0 label Sep 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression-develop Regression bug that was found on development branch, but not yet present in production release-12.6.0 Issue or pull request that will be included in release 12.6.0 Sev2-normal Normal severity; minor loss of service or inconvenience. team-confirmations Push issues to confirmations team type-bug
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants