Skip to content
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

Invalid normalization of exchange rate #7770

Closed
MrNaif2018 opened this issue Apr 20, 2022 · 7 comments · Fixed by #7772
Closed

Invalid normalization of exchange rate #7770

MrNaif2018 opened this issue Apr 20, 2022 · 7 comments · Fixed by #7772

Comments

@MrNaif2018
Copy link
Contributor

MrNaif2018 commented Apr 20, 2022

>>> Decimal("40252.918")
Decimal('40252.918')
>>> Decimal(40252.918)
Decimal('40252.9179999999978463165462017059326171875')

Hi! I wonder, why does electrum convert rates got from coingecko API this way? They are returned as floats (with correct number of digits, so assume it is like a string, but in float format). But when casting float to Decimal it uses it's binary representation, which makes no sense for a currency with 2 digits. Maybe we could use Decimal(str(price)) instead of just Decimal(price)? I think this applies to most exchange rates providers, but if it were fixed for coingecko and coindesk it would be fine for me

@MrNaif2018 MrNaif2018 changed the title Invalid rounding of exchange rate Invalid normalization of exchange rate Apr 20, 2022
@ecdsa
Copy link
Member

ecdsa commented Apr 20, 2022

where exactly does it happen?

@MrNaif2018
Copy link
Contributor Author

It is the value stored in the exchange_quotes variable of FxFetcher (or how it's called). And when calling exchange_rate() method it returns a Decimal with the rate, but it has many more numbers after the dot than needed (for usd for example, usually you would have 2-3 decimal places, not ~= 30)
This is because of binary representation of floats
If to cast it to string and then to Decimal, at least for coingecko it seems to work

@SomberNight
Copy link
Member

@ecdsa the root cause is in the exchange-specific json-parsing, e.g.

result = {ccy: Decimal(json['bpi'][ccy]['rate_float'])}

return dict([(ccy.upper(), Decimal(d['value']))

note that the historical rates are also affected, sort of independently
ExchangeBase.history contains floats (and so do the persisted exchange rate files in the cache/ folder)

@SomberNight
Copy link
Member

note that the historical rates are also affected, sort of independently

Note that CoinGecko specifically, for historical prices, sends ~max resolution floats, e.g.:

    [
      1650326400000,
      40833.5379650337
    ],
    [
      1650412800000,
      41498.12244669832
    ],
    [
      1650461427000,
      42049.45714650593
    ]

but anyway, @MrNaif2018 is right that we should just convert the raw data to string first

@SomberNight
Copy link
Member

@MrNaif2018 how did you notice in the first place? the effect should be hidden from the GUI/etc AFAICT, as most methods go through ccy_amount_str, which rounds and returns a truncated str.

Anyway, the types the internal methods expect/return are quite messy and the faulty float conversion might still somehow leak out, so I've tried to fix this and clean it up a bit at the same time.
see if #7772 looks ok

@MrNaif2018
Copy link
Contributor Author

I noticed this via https://github.com/bitcartcc/bitcart/blob/91b4ca933d474b137b5e6c876315dc93840752fe/daemons/btc.py#L438-L444
:D
When calling our exchange rate function I have seen this for ages but never got to think why is it so. Then when I found that our invoice amounts calculation code is a bit wrong (I mean, that exchange rate messed up amount calculation), then got to think and found this out

Will test it now

@MrNaif2018
Copy link
Contributor Author

Can confirm, it works now
изображение

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants