Sylius: [RFC] Direct/Inverse Exchange Rates

[venyii]

After reading the documentation and working on the code (fixture to create exchange rates automatically based on currencies), I'm not sure how to handle Exchange Rates. From my standpoint, I'd like to have full control over every exchange rate and their ratios. For example:

• EUR _(source)_ -> USD _(target)_ 1.14675
• USD _(source)_ -> EUR _(target)_ 0.8080
• EUR _(source)_ -> SEK _(target)_ 10.208 (omit SEK -> EUR for inverse calculation)

Currently this is limited to EUR->USD 1.14675 and Sylius won't allow USD->EUR to be specified (The currency pair must be unique), it will always calculate the inverse exchange rate to calculate the price USD -> EUR. From what I read this isn't always the case since the bank may offer different ratios for a currency relation.

From a data schema point of view, it's not a problem to store (which is why I didn't see a problem in the beginning, since creating/inserting models works nicely), but this came up while I was testing my code and got a More than one result was found for query although one row or none was expected. in ExchangeRateRepository::findOneWithCurrencyPair.

I found the PR (https://github.com/Sylius/Sylius/pull/6781) this was originally built (the way I imagined it), which was changed shortly after without (visible) discussion based on a single review comment https://github.com/Sylius/Sylius/pull/6781#discussion_r89059721.

So my question is: Is the current solution how it was/is intended and everything else should be customized?

Based on the answer, we should either

• change the feature to be more flexible...
  
  
  
  • Return direct exchange rate relation first in repository and if it doesn't exist, use the inverse one
  
  
  • UniqueCurrencyPairValidator checks only source -> target and not inverse
  
  
  • TBD maybe something I'm missing?
  
  
• and/or make it clear in the documentation how Sylius handles exchange rates.

But before I change anything, I'd like to hear your opinion on this.

[stale[bot]]

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in a week if no further activity occurs. Thank you for your contributions.

[vvasiloi]

@venyii I think this limitation is redundand.
The following behavior is better in my opinion:

• can create a source -> target exchange rate
• can create a target -> source exchange rate
• if the direct one is not available, then fallback to the reverse one

And it will not introduce a BC break.

[stale[bot]]

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in a week if no further activity occurs. Thank you for your contributions.

[pamil]

Thanks for your patience!

I'd go for dropping the unique constraint for inversed exchange rates. It should be possible to have both EUR/USD and USD/EUR exchange rates. To preserve backwards compatibility, when doing the conversion from EUR to USD, it should first look up for EUR/USD and then fallback to USD/EUR.

I would be happy to merge a PR that does that and includes tests for the added behaviour.