Kraken: Action Types in CSV Exports

[Griffsano]

This PR addresses #97 (but does not resolve all points mentioned in the issue).

@oldgitdaddy
Feel free to use this version for your Kraken exports. Let me know if you run into any problems.
If you have trades in USD, you can add this line in kraken_pair_map in core.py:

    "USDEUR": "USDTEUR",

The virtual sell feature may not work properly right now, so I recommend to disable CALCULATE_VIRTUAL_SELL in the config.ini. The problem is that no valid trades may be returned from the API for the current timestamp. Alternatively, a workaround for this is to use an older timestamp in https://github.com/provinzio/CoinTaxman/blob/main/src/taxman.py#L227.

I introduced a new refids list to track duplicate deposits/withdrawals and the additional deposit/withdrawal entries for staking / unstaking / staking reward actions. By tracking the Ref-IDs, duplicate entries can also be correctly identified when e.g. multiple deposits arrive in a short period of time (I think "overlapping" deposits/withdrawals could have been a problem before).
With this implementation, all first entries of deposits/withdrawals are ignored, and only the second entry leads to an operation. This way, the additional deposit/withdrawal lines for staking / unstaking / staking rewards are ignored.

The question is which deposit/withdrawal to ignore for "real" deposit/withdrawal actions? The current approach in the main branch considers the events on the blockchain as taxable (first deposit / second withdrawal). However, this could lead to erroneous FIFO/LIFO calculations e.g. if trades occur during the deposits/withdrawals (see #57). As I understand, the assets are only available for trading after the second deposit and before the first withdrawal.
@shredEngineer What do you think?

[oldgitdaddy]

Thanks @Griffsano! I will be happy to check the changes, whenever they are in master. Can anybody please merge the PR?
Thanks!

[provinzio]

As you mentioned the error, I would appreciate if you could test this branch with your full data set @oldgitdaddy

[oldgitdaddy]

Ok. I will do the testing today and let you know.

[oldgitdaddy]

The fix appears to work. It generated the tax evaluation for 2021. From my side this PR can be merged. Thanks!

[Griffsano]

Update: The mapping from USDEUR to USDTEUR is not required anymore, since the inverse coin pair EURUSD is fetched now. Additionally, virtual sells should work now.

[provinzio]

Hey @Griffsano,
I wish you a Happy New Year. :)
You did a great job, providing a hotfix for our user!

I noted some points in the code and have a question on how you treat price-fetching for virtual sells.
I am looking forward to your response, so that we can merge this asap.

[Griffsano]

Hey @provinzio, I've refactored the logic using a class variable defaultdict[str, defaultdict[str, Any]]. In the nested defaultdict, there's now a a flag for storing if the operation has been appended already (None, False, True). In addition, only the data of the first operation is stored now (it's not necessary to store all operations, since only two should occur anyways).
I'm not so familiar with defaultdict (or with Python at all, for that matter :D). Is this what you had in mind?

Additionally, I've made some changes to the get_price_kraken function: Now pairs that returned an invalid arguments error are stored and not tried again, effectively cutting the API calls for flipped pairs in half.

[provinzio]

Awesome idea to remember the invalid pairs.

defaultdicts are dictionaries which automatically initialize values for new keys.

Let me show you this weird example, why defaultdicts can make your code cleaner. This is a function which creates a dict with keys 0 to 2. All keys store values from 0 to 2 which gets added incrementally.

Doing this with dict you have to check first, wether the key is already in the dictionary. Otherwise you have to create the list first.

d: dict[int, list] = {}
for i in range (3): for j in range(3):
  if i not in d:
    d[i] = []
  d[i].append(j)

A defaultdict does exactly that for you. You don't have to check whether the key already exists in d. If a new key is requested from a defaultdict, it starts by creating a new value depending on the factory function (in this case list())

import collections
d: dict[int, list] = collections.defaultdict(list)
for i in range 3: for j in range(3):
  d[i].append(j)

[Griffsano]

I've made the changes with storing the operation classes. However, I'm not happy yet with the implementation of the new create_operation / append_created_operation functions.

[provinzio]

Append operation function should use the new create operation function.

I propose that we distinguish between create/append operation function which get the raw data and create/append the operation and an helper function _append_operation which is nothing else than self.operstions.append

[Griffsano]

Like this? There are still some mypy errors, not sure how to resolve them. The problem is that we want to return any subclass of Operation:

src/book.py:74:9: error: Returning Any from function declared to return "Type[Operation]"  [no-any-return]
src/book.py:104:32: error: Argument 1 to "_append_operation" of "Book" has incompatible type "Type[Operation]"; expected "Operation"  [arg-type]
src/book.py:665:52: error: Argument 1 to "_append_operation" of "Book" has incompatible type "Type[Operation]"; expected "Operation"  [arg-type]
Found 3 errors in 1 file (checked 12 source files)

And we probably also want to add collections to the requirements.txt.

[provinzio]

Collections is a default python module

[Griffsano]

Alright, then we have closed all the open review items. I think it can be merged, what do you think?

[provinzio]

Good job so far 👍 I am currently not at home and can not review your code properly. I'll set the review on my to-do list.