jamsix / ib-edavki

Skripta, ki prevede XML poročilo trgovalnih poslov v platformi InteractiveBrokers v XML format primeren za uvoz v obrazce Doh-KDVP, D-IFI, Doh-Div in Doh-Obr v eDavkih Finančne uprave.
MIT License
173 stars 57 forks source link

Fixes and enhancement: Fix broken getIbAffiliateInfo function, Broker Interest Received fix (CYEP and SYEP was not handled), FUND asset category handling and skipping of unsupported asset categories #125

Closed ZigaSajovic closed 4 months ago

ZigaSajovic commented 4 months ago

With the call of the taxman, I return this year again (https://github.com/jamsix/ib-edavki/pull/101) to contribute.

This pull request contains fixes and enhancements:

  1. Fix the broken getIbAffiliateInfo function
  2. Fix handling of interest paid by IBKR on their CYEP and SYEP programs (interest paid from both was excluded from generated reports)
  3. Add handling of mutual funds
  4. Add skipping of unsupported asset categories with an issued warning

See details for each bellow

Fixes

Broken getIbAffiliateInfo function

The function getIbAffiliateInfo is broken. As it currently stands, the function aims to perform a linear-search over the list, but it terminates if the first element does not match the predicate. In other words, the original implementation only checks the first element in the ibAffiliateInfos list. This caused the script to produce empty affiliate info fields for any user not under the affiliate found first in the file.

This is fixed by moving the return outside of the for loop iterating the list. See the diff here https://github.com/ZigaSajovic/ib-edavki/commit/317ce711f21f17c0f79b21d44c35cb0153699f25

Broker interest received

As it stood before this pull request, the generator for Doh-Obr checks for

ibCashTransaction.get("type")  in ["Broker Interest Received"]

This is not entirely correct. IBKR labels all its payments as Broker Fees. This includes things like their Cash Yield Enhancement Program ( description: EUR CYEP INCOME FOR XXX) and their Stock Yield Enhancement Program (description: EUR IBKR MANAGED SECURITIES (SYEP) FEES XXXX). For this reason all the interest paid from these two programs was excluded from the reports.

The fix is simple:

ibCashTransaction.get("type")  in ["Broker Interest Received", "Broker Fees"]

See the diff here https://github.com/ZigaSajovic/ib-edavki/commit/8525f031f0f04df017e86aececffad2288d50620

Enchancments

Adding a FUND asset category

Because a mutual fund is traded in shares and it distributes its income as a dividend, the code is fully equipped to handle them as an asset category. Hence one only has to add it

normalAssets = ["STK", "FUND"]

in the normalAssets list here.

See the diff here https://github.com/ZigaSajovic/ib-edavki/commit/7548ed47bc3921fd66889b7108c86c2f45881d35

Skipping unsupported asset categories

It is annoying that if ones flex-form contains an unsupported asset category, the script terminates and one has to go into the file and remove those entries.

The proposed enhancement works by recording all the securityIDs that belong to an asset category that is not supported in the step that detects whether it is of a normal or a derivate type. Unsupported securityIDS record a set of all the unsupported categories that were encountered for them. Than we iterate over this collection and remove all trades for the recorded securityIDs. We issue a warning to the user with the full info of what was skipped.

Assume the user contained some bonds. Than they are issued a warning:

WARNING: We are skipping the following securities because their assetCategories are currently not supported
.....................YOU NEED TO HANDLE THEM MANUALLY!
.....................XS1893631330: BOND,
.....................US037833AS94: BOND,

But the scripts processes the rest of the data and generates reports. You can see the diff of the change here https://github.com/ZigaSajovic/ib-edavki/commit/f0b073876a9b1d830a7f74c11a6f30e08d57a442

mfilej commented 4 months ago

Nice! I tested it and I can report that for my data there is no difference in the output (which is to be expected, I think).

ZigaSajovic commented 4 months ago

@mfilej Good to hear. The difference in the output is to be expected for users who:

  1. Were enrolled in the CYEP, which is done automatically under certain conditions (cash balance greater than 10k and perhaps some NAV lower limit)
  2. Were enrolled in the SYEP, which has to be done manually.
  3. Own shares of a mutual fund
  4. Have assets that belong to a category which is currently not supported (like BOND, BILL, etc.)
pronebird commented 4 months ago

Great work!

RokLenarcic commented 4 months ago

Broker fees so lahko tudi negativne, e.g. ce imas short odprt, in ne pasejo zraven.

ZigaSajovic commented 4 months ago

@RokLenarcic Ker sam nisem imel tega primera, sem spregledal to možnost. To pomeni, da je potrebno dodati preverbo description polja, ali gre za CYEP ali SYEP.

Primer vnosa za CYEP

<CashTransaction accountId="" acctAlias="" model="" currency="EUR" fxRateToBase="1" assetCategory="" subCategory="" symbol="" description="EUR CYEP INCOME FOR JUN-2023" conid="" securityID="" securityIDType="" cusip="" isin="" listingExchange="" underlyingConid="" underlyingSymbol="" underlyingSecurityID="" underlyingListingExchange="" issuer="" multiplier="0" strike="" expiry="" putCall="" principalAdjustFactor="" dateTime="20230706" settleDate="20230706" amount="12.99" type="Broker Fees" tradeID="" code="" transactionID="534220565" reportDate="20230706" clientReference="" actionID="" levelOfDetail="DETAIL" serialNumber="" deliveryType="" commodityType="" fineness="0.0" weight="0.0" />

Primer vnosa za SYEP

<CashTransaction accountId="" acctAlias="" model="" currency="EUR" fxRateToBase="1" assetCategory="" subCategory="" symbol="" description="EUR IBKR MANAGED SECURITIES (SYEP) FEES FOR JUL-2023" conid="" securityID="" securityIDType="" cusip="" isin="" listingExchange="" underlyingConid="" underlyingSymbol="" underlyingSecurityID="" underlyingListingExchange="" issuer="" multiplier="0" strike="" expiry="" putCall="" principalAdjustFactor="" dateTime="20230803" settleDate="20230803" amount="0.17" type="Broker Fees" tradeID="" code="" transactionID="551728621" reportDate="20230803" clientReference="" actionID="" levelOfDetail="DETAIL" serialNumber="" deliveryType="" commodityType="" fineness="0.0" weight="0.0" />

S tem popravkom bi skripta delovala za CYEP in SYEP, z drugimi Broker Fees pa se ne bi ukvarjala. V kolikor se to zdi OK bom vnesel popravek.

RokLenarcic commented 4 months ago

Jst preprosto sfiltriram vn negativne zneske.

ZigaSajovic commented 4 months ago

@RokLenarcic V tem primeru pa morda že obstoječa vrstica poskrbi za to. Ali je bolje da se doda preskok že ob zaznavi tukaj?

RokLenarcic commented 4 months ago

Zgleda to poskrbi.

ZigaSajovic commented 4 months ago

Additional bug-fix

Bug

The function getIbAffiliateInfo is broken. As it currently stands, the function aims to perform a linear-search over the list, but it terminates if the first element does not match the predicate. In other words, the original implementation only checks the first element in the ibAffiliateInfos list. This caused the script to produce empty affiliate info fields for any user not under the affiliate found first in the file.

Fix

Simply move the return outside of the for loop iterating the list.

jamsix commented 4 months ago

@RokLenarcic @pronebird kak komentar na popravke? Good to merge?

pronebird commented 4 months ago

@jamsix

@RokLenarcic @pronebird kak komentar na popravke? Good to merge?

Pogledam v kratkem casu. Drugac pa poglej https://github.com/jamsix/ib-edavki/pull/127 ker delno vsebuje enako spremembo.

ZigaSajovic commented 4 months ago

Sem pogledal in #127 res vsebuje enega od popravkov (fix broken getlbAffiliatelnfo function). Katero merge-at je vaša odločitev 😊

ddbk commented 4 months ago

Fix handling of interest paid by IBKR on their CYEP and SYEP programs (interest paid from both was excluded from generated reports)

Please update Readme with instructions to include this data in the flex statement export. Following the current instructions I don't have it in my flex statement and consequently Doh-Obr isn't generated correctly even with the proposed fix.

ZigaSajovic commented 4 months ago

Fix handling of interest paid by IBKR on their CYEP and SYEP programs (interest paid from both was excluded from generated reports)

Please update Readme with instructions to include this data in the flex statement export. Following the current instructions I don't have it in my flex statement and consequently Doh-Obr isn't generated correctly even with the proposed fix.

Done

ddbk commented 4 months ago

Nice, looks good to me @ZigaSajovic! 🙌

A nice future improvement could be to change the type of interest for CYEP from type 2 to type 3 or 8 (or whichever is the correct one for this type of interest).

pronebird commented 4 months ago

Just pip-installed this PR via pip3 install git+https://github.com/ZigaSajovic/ib-edavki.git

It seems to crash when generating Doh-Div:

Traceback (most recent call last):
  File "/Users/pronebird/Library/Python/3.8/bin/ib-edavki", line 8, in <module>
    sys.exit(main())
  File "/Users/pronebird/Library/Python/3.8/lib/python/site-packages/ib_edavki.py", line 1199, in main
    closestDividend["taxEUR"] += closestDividendTax / getCurrencyRate(
UnboundLocalError: local variable 'closestDividend' referenced before assignment
ZigaSajovic commented 4 months ago

Just pip-installed this PR via pip3 install git+https://github.com/ZigaSajovic/ib-edavki.git

It seems to crash when generating Doh-Div:

Traceback (most recent call last):
  File "/Users/pronebird/Library/Python/3.8/bin/ib-edavki", line 8, in <module>
    sys.exit(main())
  File "/Users/pronebird/Library/Python/3.8/lib/python/site-packages/ib_edavki.py", line 1199, in main
    closestDividend["taxEUR"] += closestDividendTax / getCurrencyRate(
UnboundLocalError: local variable 'closestDividend' referenced before assignment

Can you provide an annonimized input that caused the crash? @pronebird

ZigaSajovic commented 4 months ago

@pronebird This seems to be a bug that can fire even without the changes introduced here (I am not saying that the changes have no mistakes).

Given that it crashes at line 1199, it must have entered the if here https://github.com/ZigaSajovic/ib-edavki/blob/f3e45e02271e345776c5373052c4a8f0148cb5f3/ib_edavki.py#L1145. The only branch inside it that does not define the closestDividend variable is here https://github.com/ZigaSajovic/ib-edavki/blob/f3e45e02271e345776c5373052c4a8f0148cb5f3/ib_edavki.py#L1162. Did you get this printed message? I am looking at the code on my phone, but this seems to be the only way for the variable to not be defined by the time the code reaches here https://github.com/ZigaSajovic/ib-edavki/blob/f3e45e02271e345776c5373052c4a8f0148cb5f3/ib_edavki.py#L1199.

Regardless of this pull request, this should not be allowed. Meaning if the code falls in this branch https://github.com/ZigaSajovic/ib-edavki/blob/f3e45e02271e345776c5373052c4a8f0148cb5f3/ib_edavki.py#L1162 it will always crash after the branch.

pronebird commented 4 months ago

@pronebird This seems to be a bug that can fire even without the changes introduced here (I am not saying that the changes have no mistakes).

Given that it crashes at line 1199, it must have entered the if here https://github.com/ZigaSajovic/ib-edavki/blob/f3e45e02271e345776c5373052c4a8f0148cb5f3/ib_edavki.py#L1145. The only branch inside it that does not define the closestDividend variable is here https://github.com/ZigaSajovic/ib-edavki/blob/f3e45e02271e345776c5373052c4a8f0148cb5f3/ib_edavki.py#L1162. Did you get this printed message? I am looking at the code on my phone, but this seems to be the only way for the variable to not be defined by the time the code reaches here https://github.com/ZigaSajovic/ib-edavki/blob/f3e45e02271e345776c5373052c4a8f0148cb5f3/ib_edavki.py#L1199.

Regardless of this pull request, this should not be allowed. Meaning if the code falls in this branch https://github.com/ZigaSajovic/ib-edavki/blob/f3e45e02271e345776c5373052c4a8f0148cb5f3/ib_edavki.py#L1162 it will always crash after the branch.

Yeah it craps out after "Cannot find a matching dividend for INTC ... ". I am not sure why I see this print because all I did was generate a new XML report based on updated instructions. Perhaps I forgot something. I am trying to diff two of my XML files and see what's missing but the tooling here ugh.. give me a bit of time and yes I agree your changes don't affect the bug I suppose.

ZigaSajovic commented 4 months ago

@pronebird

Good to know my armchair analysis was correct. This behavior still needs to be addressed. Should I open an issue for it in which we can discuss what the best behaviour should be? We can either exit with an error, or skip the transaction and report a warning to the user. I will use this script for a long time so I'd gladly fix this bug as well.

ZigaSajovic commented 4 months ago

@pronebird I think you encountered this issue here https://github.com/jamsix/ib-edavki/issues/117

pronebird commented 4 months ago

@ZigaSajovic yeah I think it would be great to keep track of.

This is what it prints to me:

Cannot find a matching dividend for INTC(US4581401001) CASH DIVIDEND USD 0.365 PER SHARE - US TAX (20230301;202000) of -1.37.

I checked my other XML export and I don't have one transaction that has "Ordinary Dividend" in it. I am gonna regenerate XML and see if it does the trick. Probably forgot to tick some boxes in new flex report.

Update: my bad, forgot to tick dividends in flex report. Been a long day.

ZigaSajovic commented 4 months ago

@pronebird Well, the script should inform you of this mistake and not crash. The message should be more explicit as we know what the problem is and than abort execution. So it's good that this happened. I will fix this after this PR is merged. Thanks for looking into it.

pronebird commented 4 months ago

It worked. I had some bug on IB, couldn't enable dividends and had to trash the flex report and create a new one, then it worked.

Well this is nice:

WARNING: We are skipping the following securities because their assetCategories are currently not supported
YOU NEED TO HANDLE THEM MANUALLY!
US912797FL60:  BILL, 
US912797JF56:  BILL, 

Apart from thatDoh-Obr with monthly interest was generated too. Seems to be correct based on a brief look at XML entries. Great work! 👍

jamsix commented 4 months ago

It's been a long day, but I trust @pronebird 100%.

Some amazing work here @ZigaSajovic, thanks!