Improvement: IBFlex support cross-currency dividend import

[georgemac-labs]

Note: depends on #5291

I have identified a problematic scenario when using the IBFlex importer. Example:

• Portfolio base currency is EUR
• Security is quoted in GBP
• Dividend is paid in USD

Result: import of the dividend transaction fails, since the IB export only contains ConversionRate elements for the base currency – PP finds no rate to convert the USD dividend to GBP.

However, in my export files, the CashTransaction element representing the dividend payment does contain the EUR/USD rate in fxRateToBase. Thus it is, in fact, possible to calculate the GBP value of the USD dividend, using the EUR/USD and EUR/GBP rates.

A further complexity is that the security could also be quoted in a minor unit such as GBX, requiring an additional fixed-rate conversion (USD→EUR→GBP→GBX!).

This PR extends the importer to handle both cases. It depends on #5291 to handle the minor currency unit case.

Both of these occur in my personal data, and that is the basis for the new tests in the PR. With the code changes, my real-world transactions import correctly.

[buchen]

First, FYI, I have force pushed a rebase of this pull request into your branch. That was needed because I merged #5291 but renamed the method and did some minor changes (see the comment in the commit message).

[buchen]

I think I understand what you are trying to achieve. I have two things I believe we should discuss:

• First, there is now multiple code places that are using fxRateToBase without the previous check for the currency. I do not really understand why this does not have to be qualified with a currency check.
• Second, the code is now using one (random) fixed currency pair out of potentially many. I think the code should first check, if it can use the "bridge" via the account currency. If that is not possible, then iteratively check if an additional hop via a fixed currency pair is possible. I think that could also reduce the code duplication in the method.

And one more thought: the feature now tries to add "hops" in the currency conversion. The ExchangeRateProviderFactory uses Dijkstra to find the shortest path between any of the known pairs. I wonder if it makes sense to reuse this here: throw in all known exchange rates from the file + the fixed exchange rates and resolve it. It could be more generic than having a limited implementation that tests whether one hope can fix the problem.

@lmb - I would be interested in your opinion as you contributed the previous work to determine a "fallback" exchange rate.

[buchen]

I am not sure why it is possible to use fxRateToBase in an unqualified manner. The existing code (line 930) was already checking this property. Wouldn't we need to check the currency pair here (and wouldn't that be the account currency as above?)

[buchen]

This is returning the first out of many possible pairs. For example, for EUR there are multiple fixed currency pairs configured. This code would take one of them and go with it. I believe logically it has to return a list of currencies.

[buchen]

Again, this is doing an unqualified check - doesn't this have to somehow understand/check into which currency this converted?

[lmb]

Haven’t had a chance to look at the code yet, comments are based on the discussion.

• fxRateToBase always needs to take base currency of the account into account. It’s not always possible to infer this from the xml.
• It would be great if exchange rate handling reused some existing code. I couldn’t find the right thing when I wrote the previous patch.
• I suspect reason that @georgemac-labs import fails is because the XML is missing the conversion rates section, see Document the IB Flex importer portfolio-help#183 you can add this to your flex query to work around the issue for now.

Instead of complicating the conversion logic my preference would be that cross currency imports require the conversion rates as they do now, and that we somehow surface that better in the UI. Maybe we can use the errors system?

[georgemac-labs]

Thanks for the comments – I will look into them.

@lmb see description above: I have the currency rates, that is not the problem. IBKR only gives you rates between your base account currency and others. So in my case, I have rates for EUR to x.

In the scenario I encounter, the security is denominated in a foreign currency (GBP) AND the dividend is paid in another foreign currency (USD). Ergo, there is no suitable rate in the file and only way to get the rate is indirectly.

[lmb]

Why doesn't

portfolio/name.abuchen.portfolio/src/name/abuchen/portfolio/datatransfer/ibflex/IBFlexStatementExtractor.java

Lines 928 to 949 in 88d8792

	if (accountCurrency != null)
	{
	if (toCurrency.equals(accountCurrency) && element.hasAttribute("fxRateToBase"))
	{
	// Avoid cross rate if possible by using fxRateToBase from
	// the
	// transaction element itself.
	return asExchangeRate(element.getAttribute("fxRateToBase"));
	}
	// Attempt to calculate cross rate via accountCurrency. No use
	// in trying a different intermediate currency, it seems like
	// toCurrency is only ever the account's base.
	Pair<String, String> fromKey = new Pair<>(dateStr, fromCurrency + "-" + accountCurrency);
	Pair<String, String> toKey = new Pair<>(dateStr, toCurrency + "-" + accountCurrency);
	BigDecimal fromRate = conversionRates.get(fromKey);
	BigDecimal toRate = conversionRates.get(toKey);
	if (fromRate != null && toRate != null)
	return fromRate.divide(toRate, 10, RoundingMode.HALF_DOWN);
	}

take care of this for you? It's meant to convert via base currency. Maybe you aren't including account information in the export?

[georgemac-labs]

@lmb I think you are right here. Testing suggests the code can handle the mere absence of a direct rate. I actually had the more complex case – security in a minor unit (GBX), requiring USD→EUR→GBP→GBX. That case genuinely doesn't work, but I wrongly concluded it was a bigger problem.

I think I'd close this PR and create a new one for the minor unit case, since I'm going to be making significant changes to name, description and code! Before I do that, can I get some input on approach from you and/or @buchen, if possible?

Looks to me like a search algorithm would make sense here. I could try to use ExchangeRateProviderFactory, but it requires a Client and dependency injection, and the Dijkstra class is also private. So my idea would be to implement BFS, DFS or Dijkstra in the importer. I would go for BFS, because I don't see any weights in this scenario, but maybe I'm missing something. Thoughts?

[lmb]

I had a brief look at CurrencyConverterImpl, ExchangeRateProviderFactory, etc. Some thoughts:

1. I don't like that CurrencyConverterImpl falls back to a 1:1 exchange rate when no matching series can be found. For the extractor it makes more sense to error out instead of creating lots of transactions with bogus conversions which are hard to fix later on.
2. ExchangeRateProviderFactory seems to use a global list of exchange rate providers to find the rate. I'm not sure whether this is the right thing to do. Using only IB provided rates seems to be "safer" in the sense that it might make it easier to match transaction amounts to IB statements? Not entirely sure this is correct. In any case, IB seems to provide a lot of rates all going to the base account currency, even for currencies not involved in transactions at all.

I see two options, not sure which is better:

• Refactor the codebase in a way where we can use ExchangeRateTimeSeries(Impl) directly, without going via global providers. Seems like overriding getAvailableTimeSeries() in ExchangeRateProviderFactory is a good starting point for this.
• Add a "temporary import provider" which we can somehow add to the Factory temporarily, which contains the exchange rates from the XML. That way "missing" exchange rates are backfilled from global providers.

In any case there needs to be CurrencyConverterImpl.getRateWithoutFallback() or similar.

Personally I wouldn't re-implement any of this inside the extractor. Less code is better.

[georgemac-labs]

@lmb am I understanding correctly: the first idea is using ExchangeRateTimeSeries with the Flex Query rates data?

In all the cases I've seen, if your IB Flex Query is set up correctly, you have all the exchange rates you need for the import. Only the major/minor unit relationships are missing. So combining that with data from other providers seems like unnecessary complexity?

I agree that code duplication is undesirable, but I see it as a tradeoff rather than an absolute rule. In this case, I'm wondering how much work it might cost to avoid 50 lines of fairly boilerplate BFS in the importer :)

[buchen]

Only the major/minor unit relationships are missing.

I understand this as "missing are GBX to GBP and vice versa" because IB assumes that a user can simply convert.

If this is "only" about the major/minor relationships, then using the ExchangeRateProviderFactory seems too big indeed.

To be honest, I think we should also consider to implement support only for GBX to GBP. Again, the other fixed rates such as DEM-EUR or USD-AED seem unlikely pairs where IB assumes the rate just exists. Simplifying for the GBX/GBP pair would be significantly easier.

Then my naive understanding for the logic inside the getExchangeRate(fromCurrency, toCurrency) is something along these lines:

1. if conversionRate contains the from-to pair, use it [today]
2. if conversionRate contains the inverse, use it [today]
3. if it has an toCurrency equals accountCurrency and fxRateToBase exists, use it [today]
4. check if fromCurrency is either GBX or GBP, convert, and look for rate along steps 1-3 again
5. check if toCurrency is either GBX or GBP, convert, and look for a rate along steps 1-3 again

[georgemac-labs]

@buchen I think that's a sensible solution for this case right now. I would also include ILS/ILA and ZAR/ZAC, because apparently those are two further cases. I don't know how likely they are in PP's userbase, but they're easy to include. If nobody has a differing opinion, I'll create the PR sometime soon.