fix: handle low numbers when saving default advanced gas fees #22790
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. |
vinistevam
changed the title
vinistevam
added
the
team-confirmations-system
Builds ready [38d63f8]
Page Load Metrics (830 ± 48 ms)
Bundle size diffs
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #22790 +/- ##
===========================================
- Coverage 68.53% 68.53% -0.00%
===========================================
Files 1089 1089
Lines 42966 42972 +6
Branches 11440 11440
===========================================
+ Hits 29446 29448 +2
- Misses 13520 13524 +4 ☔ View full report in Codecov by Sentry. |
|
LGTM! |
|
@vinistevam Build from 38d63f8a9cf05be24c39d1495a0236b0b7e71b96] - it won't let me type 0 after decimal point: decimal.movI can cut and paste 0.000000001 but it results in |
| @@ -19,6 +19,7 @@ import { bnGreaterThan, bnLessThan } from '../../../../../helpers/utils/util'; | |||
| import { useAdvancedGasFeePopoverContext } from '../../context'; | |||
| import AdvancedGasFeeInputSubtext from '../../advanced-gas-fee-input-subtext'; | |||
| import { decGWEIToHexWEI } from '../../../../../../shared/modules/conversion.utils'; | |||
| import { Numeric } from '../../../../../../shared/modules/Numeric'; | |||
|
|
|||
| const validatePriorityFee = (value, gasFeeEstimates) => { | |||
| if (value < 0) { | |||
There was a problem hiding this comment.
If I read this right, value is actually string here both before and after this change - would it make sense to treat it as a Numeric here for consistency, either by transforming it here or in the calling scope?
Then the comparison functions from Numeric can be used and the import of bnGreaterThan, bnLessThan could be removed from this file.
There was a problem hiding this comment.
Yeap apart from the FormField setting the value as number the rest of the code treating as a string. I'm pushing the changes to treat it as Numeric in both of the components for consistency as you suggested and remove the bnGreaterThan, bnLessThan.
There was a problem hiding this comment.
Ah, I missed the part where the input is still getting transformed via number through the FormField. This means the resulting value can be different than entered by the user through loss of precision from inherent limitations of floating-point representation. If we don't want to introduce rounding errors, it would have to go straight from string to Numeric without intermediary parseFloat.
Some examples of value that will be transformed: 1234.00000000000030, 1234.00000000000080.
Seems to be relevant for all places that accept decimal input via the numeric input component.
// detect discrepancy introduced through `parseFloat`
new Numeric(parseFloat(s), 10).toFixed(16) !== new Numeric(s, 10).toFixed(18)
There was a problem hiding this comment.
We should be ok with rounding numbers up to 1 Wei (0.000000001 Gwei).
I agree this is relevant for every piece that uses this component with decimals. I need to check maybe the FormTextField component doesn't have this limitation. The only issue I saw when changing to string in the component is the visual, so UX would also need to be involved.
Here is the ticket to track the precision issue.
This fix ensures that we can handle low numbers when checking the "Save these values as my default..." option.
Builds ready [dabd239]
Page Load Metrics (861 ± 25 ms)
Bundle size diffs
|
Builds ready [9e0a0ae]
Page Load Metrics (783 ± 19 ms)
Bundle size diffs
|
...onents/app/advanced-gas-fee-popover/advanced-gas-fee-inputs/base-fee-input/base-fee-input.js
Outdated
Show resolved
Hide resolved
...s/app/advanced-gas-fee-popover/advanced-gas-fee-inputs/base-fee-input/base-fee-input.test.js
Outdated
Show resolved
Hide resolved
Builds ready [49435c8]
Page Load Metrics (770 ± 19 ms)
Bundle size diffs
|
vinistevam
force-pushed
the
fix/handle-small-number-save-advance-gas-fees
branch
from
b3f28fb
to
af27baf
Compare
Builds ready [01352bb]
Page Load Metrics (794 ± 27 ms)
Bundle size diffs
|
|
Low values can be entered and saved correctly. 123.movFirefox 122.0 456.mov |
vinistevam
force-pushed
the
fix/handle-small-number-save-advance-gas-fees
branch
from
01352bb
to
ef79d2a
Compare
Builds ready [4491d22]
Page Load Metrics (1235 ± 168 ms)
Bundle size diffs
|
Builds ready [728b59c]
Page Load Metrics (936 ± 76 ms)
Bundle size diffs
|
Builds ready [bdaca06]
Page Load Metrics (1072 ± 86 ms)
Bundle size diffs
|
metamaskbot
added
the
release-11.12.0
gauthierpetetin
added
the
Sev1-high
Description
This PR aims to fix a bug when a user saves very low default values, in the example
0.000000001number was turned into1e-9string and thrown when converting it down the line in the next transaction.Related issues
Fixes: #22605
Manual testing steps
Max Base Fee1andPriority Feeto0.000000001Save these values as my default for the <chain name> network.I was able to reproduce the issue using Ganache with default gas zero.
Screenshots/Recordings
Before
before_fix_gas.webm
After
advance-gas.webm
Pre-merge author checklist
Pre-merge reviewer checklist