fix: DarthCoin rant #2898
fix: DarthCoin rant #2898
Conversation
nicolasburtey
requested review from
sandipndev and
UncleSamtoshi
and removed request for
sandipndev
|
The force is strong with this one |
|
i approve of this message. |
nicolasburtey
force-pushed
the
feat--saving-unit-of-account-when-adding-an-amount
branch
from
63bbe9b
to
c589d38
Compare
| } | ||
| }, [secondaryNewAmount, setNumberPadAmount, client, swapDefaultUnitOfAccount]) | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
Does this cause a flicker allowing it to render the initial amount and then on the next render switching to the default? I wonder if the change needs to be made before getting to this component, so that the first time it renders it is rendering the correct money amount.
There was a problem hiding this comment.
I have not seen any flicker in my simulator. I think there were to do some calculation before rendering but I don't remember what it was anymore, I'll look for it
There was a problem hiding this comment.
nothing obvious coming to mind from my convo with GPT4. let's merge and see how it behaves and this is an issue?
| secondaryNewAmount && | ||
| (() => { | ||
| const client = useApolloClient() | ||
| const swapDefaultUnitOfAccount = |
There was a problem hiding this comment.
I think this implementation may have a bug. Its not a given that the primary amount is always the display currency or always the settlement currency. If swap was set to true and one use of the component passes in a display amount and another use of the component passes in a settlement amount, they would both end up swapping . A more accurate name would be useDisplayAmountAsInitialAmount and then you could explicitly handle the amount currencies before they reach this component.
There was a problem hiding this comment.
in what scenario would we not have a display currency or settlement currency? ie: what would make it that the order would be different?
There was a problem hiding this comment.
If on receive screen the initial primary amount was set to 0 Display as the default and if on the send flow the primary amount was set to 0 BTC as the default. This isn't what we do now, but we dont have anything in place to prevent someone from misusing the component.
Right now I believe we leave the amounts as undefined and then the initial amount is set in this line https://github.com/GaloyMoney/galoy-mobile/blob/c7c81a1f9d2df633e76752b964ab1a82555c3041/app/components/amount-input-screen/amount-input-screen.tsx#L147 with the zeroDisplayAmount. We could get rid of needSwapCurrencyInit and in this line use the value useDisplayAmountAsDefaultAmount to decide between zeroDisplayAmount and zeroSettlementAmount as the default.
|
I just want to make some additional comments here, to clear up a bit. I myself I did several times the mistake to input let's say 10000 in that field forgetting that is set automatically to EUR/fiat. If I could make that mistake, imagine how a total noob could do it. And this can end up in big mistakes. If I set Blink in settings to display in BTC, I suppose to be displayed on all screens in BTC and not in fiat. Including the main top screen, that it display ONLY in fiat. Make it optional: who wants to deal in fiat, fine, display in fiat all. But also I want to use it only in sats. I don't care too much about fiat. I hope this comment make some clarifications and make your life easier with the changes. |
Actually the reason we have fiat as default is because we consider the noob scenario as a key thing here. noob will always set their price in fiat term, so if we always have fiat by default, they're less likely to enter an amount under a wrong denomination my assumption is this is a feature for a power user that want to have sats as the default unit of account when they type an amount |
|
Education of noobs start by changing their mindset, slowly, from fiat to BTC. Who wants to live in the fiat standard, fine. They can keep displaying fiat. |
aka: saving unit of account used when typing an amount