fix: send tokens with decimals > 20 #21147
Conversation
|
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. |
|
I have read the CLA Document and I hereby sign the CLA |
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #21147 +/- ##
========================================
Coverage 68.64% 68.64%
========================================
Files 1017 1017
Lines 40797 40799 +2
Branches 10893 10893
========================================
+ Hits 28004 28006 +2
Misses 12793 12793
☔ View full report in Codecov by Sentry. |
Builds ready [6bb3947]
Page Load Metrics (1371 ± 310 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
| @@ -733,14 +733,14 @@ describe('SwapsController', function () { | |||
| gasEstimateWithRefund: '0xb8cae', | |||
| savings: { | |||
| fee: '-0.061067', | |||
| metaMaskFee: '0.5050505050505050505', | |||
| metaMaskFee: '0.5050505050505050505050505050505', | |||
There was a problem hiding this comment.
the swap-controller uses the Numiric module in the fetchAndSetQuotes function, which is why we get more digits here ( that's why the swap-controller was impacted ).
I wanted to know if I should introduce the decimal as a parameter of the Numeric module and create separated instances with specific config for each one in order to exclude the impact on the swap controller, and for that I want your opinion and suggestions first.
There was a problem hiding this comment.
@dan437 you are probably best to answer that one.
There was a problem hiding this comment.
I think this will affect all global usage of bignumber.js across the package, regardless of if through Numeric or not? May be missing something but don't see how one would parameterize or instantiate the module in a meaningful way..?
(This currently does not affect bignumber.js usage by dependencies, since they use different resolutions of the package. But if/when that dedupes through a bump of bignumber.js, they would then also be affected)
If you instantiate a new BigNumber constructor and exclusively use that inside Numeric, it should be possible to contain it, though, and an optional parameter could be added to the Numeric class constructor.
There was a problem hiding this comment.
On the flip side, is it actually desired to have fewer decimal places in other places? Seems more reasonable that in cases where lower precision should be displayed, that this should be taken care of in the UI (e.g. as parameter to toString() when rendering)
There was a problem hiding this comment.
hey @legobeat ,
thnx for your answer.
it's already computed in the UI in order to get lower precision.
i posted my comment here to mention that this will affect other module ( like swap-controller ) and the only way to avoid it is to use another bigNumber instance et use it exclusively in the Numeric package like you said.
but a question come to my mind , is that bad to have more precision ? i think a global change in the bigNumber module can be a good solution but i wanted to have other suggestions first.
There was a problem hiding this comment.
Lifting this to draft for now. @salimtb Feel free to undraft when there is a conclusion on what the preferred approach here is and the PR has been updated accordingly, if required.
There was a problem hiding this comment.
@legobeat i removed the draft , the conclusion was to add more precision and put max decimals to 36 ( we have the same limit in the core compoment )
|
Adding @jpuri who had a concern with this previous attempt: #14482 (comment) |
Builds ready [3bb9360]
Page Load Metrics (792 ± 370 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
This comment was marked as resolved.
This comment was marked as resolved.
Builds ready [85bb3da]
Page Load Metrics (716 ± 394 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [85bb3da]
Page Load Metrics (716 ± 394 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [85bb3da]
Page Load Metrics (945 ± 371 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [b3f9718]
Page Load Metrics (956 ± 376 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [e429962]
Page Load Metrics (1081 ± 351 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Description
Now MetaMask cannot send tokens with decimals larger than 20, this fix allows MetaMask to send tokens with tokens whose decimals <= 32.
the solution was to config
bigNumberand set decimals to 36Manual testing steps
1 - Import token with decimals > 20
2 - Try to send an amount of this token using the send button
Screenshots/Recordings
Before
before.mov
After
fix.mov
Related issues
_Fixes #14481
Pre-merge author checklist
Pre-merge reviewer checklist