Repository navigation
Conversation
Rates were stored and looked up by a key built from the numeric codes of the two currencies. CNY and CNH share the numeric code 156, so their rates were stored together and a lookup for one could return the latest rate added for the other. Cryptocurrencies have numeric codes above 999, which broke the check used by the smart lookup, so a derived rate starting from them (e.g., BTC to EUR through USD) could not be found.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ExchangeRateManagerstores and looks up rates with a key built from the numeric codes of the two currencies (min*1000 + max). This breaks in two cases:lookup(USD, CNY)returns the USD/CNH rate, and converting 100,000 USD with it gives 712,360 CNH instead of 710,120 CNY. With only USD/CNY stored,lookup(USD, CNH)returns the USD/CNY rate instead of failing.hashes(), which decodes the key with% 1000and/ 1000, never recognizes a rate involving them. As a result, a derived rate can't start from them: with BTC/USD and EUR/USD stored,lookup(EUR, BTC)works butlookup(BTC, EUR)fails with "no conversion available from BTC to EUR".The key is now the pair of currency codes, in alphabetical order, and the smart lookup also uses the codes to avoid cycles. Only private members change.
The two new test cases in
ExchangeRateTestsfail on master as described above and pass with the change; the other exchange-rate and money tests and the full test suite pass.