fix(frontend): incorrect "value" type in InputCurrency #3592
Conversation
|
@peterpeterparker @AntonioVentilii please take a look at the PR's description and code. |
DenysKarmazynDFINITY
force-pushed
the
fix(frontend)/input-currency-issue
branch
from
cc232e7
to
2ff8fb1
Compare
DenysKarmazynDFINITY
force-pushed
the
fix(frontend)/input-currency-issue
branch
from
2ff8fb1
to
2f948a0
Compare
| import { assertNonNullish } from '@dfinity/utils'; | ||
| import { fireEvent, render } from '@testing-library/svelte'; | ||
|
|
||
| describe('ConvertAmountExchange', () => { |
There was a problem hiding this comment.
I think here is InputCurrency, right?
There was a problem hiding this comment.
Totally right, but since we will most likely proceed with the Solution 2, this PR won't be merged.
| // Convert inputValue to number before updating parent's value | ||
| value = nonNullish(inputValue) ? Number(inputValue) : undefined; | ||
|
|
||
| // Bubble nnsInput as consumers might require the event as well |
There was a problem hiding this comment.
not really sure we need this comment: I think it is a common pattern in the code of Oisy
|
Wow! Good catch, it's been there forever! Actually, I rather like solution 2. Because properties can flow up and down, I feel like it's more maintanable if we can avoid having to manipulate it therefore, I gave it a quick try and, at least from a linter perspective, it doesn’t seem too bad to implement, as I managed to put it together quickly in #3609. WDYT? |
|
@peterpeterparker yeah, solution 2 also work for me perfectly fine, thanks for preparing the PR! I |
Cool, then let's proceed with solution 2. |
# Motivation "Solution 2" of #3592 - i.e. @DenysKarmazynDFINITY spotted a bug that was there since ages. We interpreted the value of the `Input currency` as `number | undefined` while actually the type is a `string | number | undefined`. # TODO Create a type `type OptionCurrencyValue = string | number | undefined` or something. # Changes - Property `value` in `InputCurrency` must be of type `string | number | undefined` - i.e. `string` was missing - Populate changes in consumers - Add `Number()` cast to few utils when property is defined
Motivation
While working on the conversion flow, I noticed one suspicious thing: the
valueprop that is controlled byInputCurrencyis actually astringand not anumberas it is expected to be according to the types in our codebase. It means that we incorrectly assume across the wholesendflow thatamountisnumber | undefinedwhen it's in realitystring | undefined. The same applies to the convert flow:sourceAmountwas never a number.After reading the GIX Input docs, I found this paragraph:
Having said that, we have quite an error-prompt situation: I noticed this issue while debugging one of the conversion services -
sendAmount === 0was always giving mefalseafter typing 0, even though TS-wise the statement looks correct. Since in the code allsendcomponents also expect it to be anumber, this may lead to the same issue.Solution 1 (implemented in this PR): as suggested by GIX, I added an internal
inputValueto theInputCurrencycomponent which hasstring | undefinedtype, and onnnsInputI convert it tonumberbefore notifying parent. Also, provided some tests to test this behaviour.Solution 2: update
amounttype everywhere where it's used. Would require a lot of changes.