Jon/fix/light-acc-default-buy #5280
Conversation
src/plugins/gui/amountQuotePlugin.ts
Outdated
|
|
||
| if (isLightAccount) { | ||
| // Round down | ||
| initialValue1 = floor(initialValue1, 0) |
There was a problem hiding this comment.
What's the point of this? The rounding should be the same for light account or not.
There was a problem hiding this comment.
See the description - in the original implementation it was possible to potentially round up, so I wanted to retain that behavior for full accounts to get a little more revenue from full accounts
There was a problem hiding this comment.
@Jon-edge so why not for light accounts?
There was a problem hiding this comment.
In the euro case, our original rounding rules rounds up to 50 euros, >$50 usd. If we're OK
with light accounts buying a little more than $50 then that would be even
better.
I'll make the change.
src/plugins/gui/amountQuotePlugin.ts
Outdated
| @@ -523,8 +523,13 @@ export const amountQuoteFiatPlugin: FiatPluginFactory = async (params: FiatPlugi | |||
| quoteFiatCurrencyCode === 'iso:USD' ? '1' : (await getHistoricalRate(`${quoteFiatCurrencyCode}_iso:USD`, new Date().toISOString())).toString() | |||
| const quoteDollarValue = mul(bestQuote.fiatAmount, quoteFiatRate) | |||
|
|
|||
| if (gt(quoteDollarValue, '50')) { | |||
| stateManager.update({ statusText: { content: lstrings.fiat_plugin_purchase_limit_error, textType: 'error' } }) | |||
| if (gt(quoteDollarValue, DEFAULT_FIAT_AMOUNT_LIGHT_ACCOUNT)) { | |||
There was a problem hiding this comment.
I see this can work since in the previous commit you floored the last decimal place instead of rounding, but this is not clean logic. This is prone to break in the future if anyone touches either code location without knowing about the other. The logic of each don't even equal each other. The other commit takes $50 converted to the fiat currency then floors it. This code takes the EXACT exchange rate conversion.
Better to create a shared function that both places can call. Something like
getDefaultFiatValue(startingAmount, isoFiatCurrencyCode, isoNow) => fiatAmount
There was a problem hiding this comment.
It was intentional to have the exact exchange rate shown because that's ultimately what we check against. We want users to be able to buy as much as possible, but for the initial value we make it look nice with a rounder number
Edit: Statement was incorrect, we're showing the rounded amount in the error message, but checking against an exact amount, to give wiggle room for the user. Moot point per latest comments.
There was a problem hiding this comment.
It's a big ugly to show a value that's down to the penny. Just match the actual default fiat amount which can both go up or down based on round instead of floor
Jon-edge
force-pushed
the
jon/fix/light-acc-default-buy
branch
from
ef6d31f
to
e3a9b00
Compare
src/plugins/gui/amountQuotePlugin.ts
Outdated
| async function getInitialFiatValue(startingFiatAmount: string, isoFiatCurrencyCode: string) { | ||
| let initialValue1: string | undefined | ||
| if (isoFiatCurrencyCode !== 'iso:USD') { | ||
| const isoNow = new Date().toISOString() |
There was a problem hiding this comment.
You need to get the date or rate once and pass this into the function each time it's used. Otherwise you may get a different exchange rate for the default vs the limit. I put this in my comment as the function declaration example.
There was a problem hiding this comment.
I just realized this can be super simplified by saving the initialValue and using that as the limit in the limit code at the bottom.
There was a problem hiding this comment.
Yeah I had it like that in a prior revision where I just reuse the initial default, but thought you wanted the most recent exchange rate each time this was called.
There was a problem hiding this comment.
I dont think I ever requested a fresh exchange rate. The only important thing is that it's consistent between the default and the limit
No longer relevant
Jon-edge
force-pushed
the
jon/fix/light-acc-default-buy
branch
from
e3a9b00
to
ebafa39
Compare
src/plugins/gui/amountQuotePlugin.ts
Outdated
| @@ -61,15 +61,18 @@ const MAX_QUOTE_VALUE = '10000000000' | |||
|
|
|||
| const NO_PROVIDER_TOAST_DURATION_MS = 10000 | |||
|
|
|||
| /** Make sure to use the same timestamp for rates throughout the lifetime of the | |||
| * module. */ | |||
| const ISO_NOW = new Date().toISOString() | |||
There was a problem hiding this comment.
Ack. Don't get the date once for the lifetime of the app. Get it at least when the plugin is started. The app could stay running for days and weeks at which point the rate could drift a lot. Once per plugin launch would be much better. Please see my fixup. It's untested so please verify. If good then merge.
Jon-edge
force-pushed
the
jon/fix/light-acc-default-buy
branch
from
cbcd3f4
to
fe5659f
Compare
Original Amount Calculations (Light Acc):
3 steps were originally performed resulting in:
Changes:
CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
noneRequirements
If you have made any visual changes to the GUI. Make sure you have: