[Bug]: Precision is lost displaying Permit value in simulation and data tree

[digiwand]

Describe the bug

formatAmount in ui/pages/confirmations/components/simulation-details/formatAmount.ts utilizes Intl.NumberFormat, which requires a BigInt or number passed to it causing a loss in precision when computing large numbers. It does not accept strings. Currently, it will display 0s for the digits after 15 digits.

we should update the logic to preserve the precision

Example:

• 30001231231212312138768 → 30,001,231,231,212,312,000,000
• 115792089237316195423570985008687907853269984665640564039457584007913129639935 → 115,792,089,237,316,200,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000

Related: #25741
Related: #25438

Expected behavior

No response

Screenshots/Recordings

Steps to reproduce

1. run yarn storybook
2. go to http://localhost:6006/?path=/story/pages-confirmations-confirm-signatures-signedtypeddatav3orv4--permit-story&args=msgParams.data:3.0001231231212312e+22
3. copy and paste the following into data

"{\"types\":{\"EIP712Domain\":[{\"name\":\"name\",\"type\":\"string\"},{\"name\":\"version\",\"type\":\"string\"},{\"name\":\"chainId\",\"type\":\"uint256\"},{\"name\":\"verifyingContract\",\"type\":\"address\"}],\"Permit\":[{\"name\":\"owner\",\"type\":\"address\"},{\"name\":\"spender\",\"type\":\"address\"},{\"name\":\"value\",\"type\":\"uint256\"},{\"name\":\"nonce\",\"type\":\"uint256\"},{\"name\":\"deadline\",\"type\":\"uint256\"}]},\"primaryType\":\"Permit\",\"domain\":{\"name\":\"MyToken\",\"version\":\"1\",\"verifyingContract\":\"0xCcCCccccCCCCcCCCCCCcCcCccCcCCCcCcccccccC\",\"chainId\":1},\"message\":{\"owner\":\"0x935e73edb9ff52e23bac7f7e043a1ecd06d05477\",\"spender\":\"0x5B38Da6a701c568545dCfcB03FcB875f56beddC4\",\"value\":30001231231212312138768,\"nonce\":0,\"deadline\":50000000000}}"

Error messages or log output

No response

Version

12.1.0

Build type

None

Browser

Chrome

Operating system

MacOS

Hardware wallet

No response

Additional context

No response

Severity

No response

[gauthierpetetin]

Can this issue be closed? (since this PR's been merged)

[benjisclowder]

Collaborative Effort Required for Root Cause Analysis (RCA) on Critical Issues

We are quickly approaching the end of the quarter and we encourage you once again to take some moments and perform RCA on this critical issue. You may do so by answering the questions below:

1. What PR fixed the issue?
2. Can you pinpoint the commit from which the issue originated?
3. Write a short explanation of the technical cause of the bug
4. How could we have avoided merging this bug? What would have had to be different about our code, tests or processes?
4.1. Were there any missing unit, e2e or manual tests that could have preempted this issue?
4.2. Were there any other elements lacking, such as typed code, comprehensive documentation, well-architected APIs, etc., that might have prevented this issue?
4.3. If your answer to a and b is no, then is there anything at all that you can think of that, if it had been different before this bug was introduced, would have prevented it from being merged?

Please provide your answers as a reply to this comment and tag me as well.

You can read more about the initiative here. Thank you!

Tagging eng. who added the fix: @digiwand / cc: @jpuri

[metamaskbot]

Missing release label release-12.1.0 on issue. Adding release label release-12.1.0 on issue, as issue is linked to PR #25968 which has this release label.

[digiwand]

Hi @benjisclowder, thanks for reaching out and the information for this! @jpuri did create the fix for the specific precision error. I do have details on its cause, so I can provide the RCA here. @jpuri feel free to edit or update this as you see fit as well

1. What PR fixed the issue?
   Fix BigNumber() more than 15 digits error: fix: Error: new BigNumber() more than 15 digits feat: apply ellipsis #25741
   &
   Fix value precision for large values: fix: change in number format to fix loss of precision for very big values #25968

2. Can you pinpoint the commit from which the issue originated?
   Generally, Redesigned Confirmations (new UI created 2024) has been built to display entire values rather than their scientific notation forms. Old confirmations would display the scientific notation form (e.g. 3.0001231231212312e+22). Since I believe the precision bug has existed since the Redesigned Confirmation's inauguration, I cannot pinpoint its exact origin commit. I can however add...
   
   BigNumber() more than 15 digits error has been a common bug that has appeared in various parts of the codebase dating back even years. There have been past workarounds which included turning inputs into strings (which may result in loss of precision) or not accepting large inputs. In this context, I think the error was created here: #25438 line 55. We added a calculation that could result in a large number being passed to BigNumber. Fixing this error uncovered the precision error.
   
   re: precision - feat: reduce the number of decimals shown for simulation detail amounts #24036 - This PR introduces the formatAmount method we use to display values for the signature redesign. Notice that in the tests, we didn't test numbers greater than the JavaScript MAX_SAFE_INTEGER. If we created tests for these large values, we might have noticed the precision error earlier on.
   
   console showing largest tested value in PR is below MAX_SAFE_INTEGER:

   > Number.MAX_SAFE_INTEGER
   > 9007199254740991
   > 1213098292340944.5 < Number.MAX_SAFE_INTEGER
   > true
   

3. Write a short explanation of the technical cause of the bug
   Please see "Technical Details" in fix: Error: new BigNumber() more than 15 digits feat: apply ellipsis #25741

4. How could we have avoided merging this bug? What would have had to be different about our code, tests or processes?
   4.1. Were there any missing unit, e2e or manual tests that could have preempted this issue?
   Yes, testing large numbers greater than MAX_SAFE_INTEGER where they may exist.
   4.2. Were there any other elements lacking, such as typed code, comprehensive documentation, well-architected APIs, etc., that might have prevented this issue?
   We have various coding patterns to handle numbers. Deciding between Numeric, BigNumber, or other number-value types isn't clear. The libraries can easily be mishandled. e.g. If a large number could be passed to BigNumber it could break the application, but there is no lint warning. As a counter-example, if a string-number value was passed to BigNumber, there would be no runtime error that could break the application.
   4.3. If your answer to a and b is no, then is there anything at all that you can think of that, if it had been different before this bug was introduced, would have prevented it from being merged?