Prevent accidentally adding base currency tx to BagFIFO

anson-vandoren commented 5 years ago

The previous check against using base currency neglected to account for the fact that the BagFIFO converts currency to uppercase, but the affected methods do not convert to uppercase before checking.

Also, found a few instances where amounts were compared to int before being converted from (potentially) string inputs.

(Failing) test cases added in the first commit, and the applicable fixes in the second commit.

bitphage commented 5 years ago

Instead of raising an exception 'Depositing the base currency is not possible.', why not just silently ignore BASE currency deposits?

anson-vandoren commented 5 years ago

Given that this is mostly used while importing from CSV files from exchanges, it's likely that if you got a wrong currency here, your whole CSV would be trying to import wrong, which would likely indicate either you chose the wrong importer, or the CSV file format no longer matches the importer.

Swallowing the error will likely lead to incorrect results down the road.

bitphage commented 5 years ago

But what if I actually deposited my base currency? I think the library needs a clear way of how to handle it.

anson-vandoren commented 5 years ago

I don't understand what you mean by depositing base currency. Depositing it where?

The BagFIFO is not set up to 'store' the base (fiat) currency, since there are no tax implications until the fiat is either:

Spent for a crypto asset (using buy_with_base_currency())
Received through a sale of a crypto asset (using pay())

In both cases, while the value is in the fiat currency, its location does not need to be tracked because no taxable events occur while it remains as a fiat currency, and there are already methods to track the input/output transactions.

Am I missing something?

bitphage commented 5 years ago

Base currency doesn't need to be stored into bag. I just mean you can deposit BASE currency into exchange account for trading. Deposit transaction may be in the transfers csv. Now such event produces an error.

anson-vandoren commented 5 years ago

hmm, OK. I can see the issue more clearly now. A couple ways we could handle it:

Change the ingestion (TPLOC for that particular csv format) to drop rows that are deposits of fiat currency. This has the benefit of being localized to a specific exchange which might include fiat deposits in their csv output (the ones I use do not do this).
Make the change you suggested to drop/ignore deposits in the base currency inside of BagFIFO. In this case, I think it should still show up as a warning in the logs

Can you give an example of an exchange that includes fiat/base deposits in their transfers csv output so I can take a look, or post some example csv output showing it?

probstj commented 5 years ago

Thanks a lot Anson for the fixes and adding the unit tests, they are great, but unfortunately, Vladimir is right, we cannot keep it just like that. I would like any future user not need to worry about it and just be able to import any csv (including base currency deposits/withdrawals) without errors, but just silently dropping data from these csvs is also not good.

So, I also though about just adding warnings to the logs (although they might and probably will be missed ;) ), which would be the simplest solution. I also like the idea of dropping the lines during ingestion (again with warnings to the logs), which would also condense the list of trades a little.

Do you think there might be any use-case involving the base currency deposits/withdrawals we might add to the lib in future? Or are there actually fees involved in withdrawing base currency? They should need to be accounted for in the capital gains.

If not, maybe the best would be to do both: Add an option to the append_..._csv method of particular exchanges, defaulting to drop rows of base currency deposits/withdrawals (then if you need to see them in an exported list of transactions, you can still opt into adding them). If they are included, log warnings when depositing/withdrawing them.

In any case, the exceptions raised in bagfifo.buy_with_base_currency and bagfifo.pay should stay, right?

anson-vandoren commented 5 years ago

OK, it sounds like we're all agreed at this point. The intent of this PR wasn't actually to add new checks, only to correct the ones that were already there (but didn't account for case correctly)

So, I'll make the change to allow deposit and withdraw with the base currency (with a log.warning) and update the tests accordingly, but keep raising an exception on buy/pay methods with base currency.

I did a bit of digging around on the fiat transfer fees on some of the more popular exchanges, and it looks like they're typically no fee for a ACH transfer into the account, but may incur a charge if using SEPA/Wire/CC to add money. What I don't know is how those fees are recorded and exported, and since I don't really want to pay a $10 fee just to get it to show up on my export CSV, not sure how to go about attacking that 😄

anson-vandoren commented 5 years ago

Actually, this will require BagFIFO.pay() to accept base currency as well, unless we require no fees for deposits/withdrawals, or the fee to be paid in a non-base currency.

For the time being (and I think this would satisfy Vladimir's use-case) we can log a warning, do nothing with the deposit/withdrawal, and move on to the next. So no accounting trail for those funds, but it won't choke on them anymore.

I think adding ability to account for fees in the base currency will take a bigger refactoring. If no objections, I'll amend this PR as noted above, and we can open an Issues discussion to decide on possibility of base-currency transactions being recorded, paid, etc.

probstj commented 5 years ago

Thanks!

probstj / ccGains

Prevent accidentally adding base currency tx to BagFIFO #17