-
Notifications
You must be signed in to change notification settings - Fork 271
Use rates server v3 endpoint #5705
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
…angeRates This breaks the snapshots until we upgrade exchangeRateActions to fix the keys in the exchangeRates object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whew, lots of changes to review here.
asset: asCryptoAsset, | ||
targetFiat: asString, | ||
isoDate: asOptional(asString), // Defaults to today if not specified | ||
expiration: asNumber |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional: Since we are updating this file format, and don't need to be compatible anymore, we could potentially convert this to a date string instead of a number. However, I do understand that the number is going to be smaller on-disk. OTOH, the number is inconsistent with the isoDate
string on the line above.
destWallet.currencyInfo.denominations[0].multiplier | ||
) | ||
), | ||
mul('5.05', div('1', polygonPrice === '0' ? '1' : polygonPrice)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Currency Conversion Logic Errors
The updated currency conversion logic has several issues. The minFeeSwapAmount
calculation is incorrect due to an inverted conversion direction or incorrect amount parameter. Fiat-to-crypto conversions in ExchangedFlipInput2.tsx
are also flawed, as getExchangeRate
provides a crypto-to-fiat rate while the division assumes the inverse. This may also cause hardcoded USD values to be misinterpreted based on the user's preferred fiat.
Additional Locations (1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved, but please check the AI comment (I think it's confused) and maybe address the two weird imports I mentioned.
@@ -8,11 +8,13 @@ import { useDisplayDenom } from '../../hooks/useDisplayDenom' | |||
import { useHandler } from '../../hooks/useHandler' | |||
import { lstrings } from '../../locales/strings' | |||
import { getExchangeDenom } from '../../selectors/DenominationSelectors' | |||
import { getExchangeRate } from '../../selectors/WalletSelectors' | |||
import { | |||
convertCurrency as convertCurrencyFromExchangeRates, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why rename the import? Was this a mistake?
@@ -8,10 +8,12 @@ import { | |||
getExchangeDenom, | |||
selectDisplayDenom | |||
} from '../../selectors/DenominationSelectors' | |||
import { getExchangeRate } from '../../selectors/WalletSelectors' | |||
import { | |||
convertCurrency as convertCurrencyFromExchangeRates, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why rename the import?
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: