provinzio / CoinTaxman

Calculate your taxes from cryptocurrency gains
GNU Affero General Public License v3.0
141 stars 31 forks source link

Parsing fails when change is equal to "0E-X" #39

Closed jhoogstraat closed 2 years ago

jhoogstraat commented 3 years ago

When change is in scientific notation, for example "0E-8", the assertion fails (book.py, line 112). This does only occur when binance produces these strange 0E-X outputs.

provinzio commented 3 years ago

The assert fails when change == 0. I thought it would never happen that the value is 0. Therefore the assert.

Would you share your statement as an example?

jhoogstraat commented 3 years ago

I have four lines in my statement that look like this: 2021-04-06 09:49:37,Spot,Transaction Related,EUR,0E-8,"" All are 0E-8 specifically.

I don't know if its related to this issue, but removing these four lines, parsing succeedes, but evaluation fails (balance_queue.py, line 95, assert not_sold > 0). Is there a documentation about the binance output? Not too easy to investigate the issue without understanding the whole codebase.

provinzio commented 3 years ago

Do you have the full history? The not sold Error will occur if you sell more coins in your statement than you bought.

It might be also related to a coin deposit from another exchange for which you have not added your statement.


I have unfortunately no idea what these 0 EUR (Transaction related) might mean. But it shouldn't make a change regarding the tax calculation. (I mean it's 0 after all).

jhoogstraat commented 3 years ago

Yeah I did move coins from Coinbase to Binance at one point.. Though the export should contain the full history. What should I do in this event?

provinzio commented 3 years ago

To evaluate your taxes, the program needs to know all of your buys/sells (especially the buy/sell time and amount). Fortunatly coinbase is also (kind of) supported by CoinTaxman. In the best case you'd have your coinbase statement. In that case, just include it, too.

If you can not access your coinbase history anymore, you have to reconstruct it somehow. Otherwhise it might be impossible to calculate your taxes.


Edit: Ah, I may have understood you wrong. So you already added coinbase and binance account statements, but it's still raising the error?

jhoogstraat commented 3 years ago

Ah no, I only used Binance's statement. It is still complicated as I used Coinbase and Coinbase Pro...

provinzio commented 3 years ago

There is a PR implementing Coinbase Pro support (#31). It might be already functional, you might want to check that out.

jhoogstraat commented 3 years ago

Ah nice! Sadly it does not work as indended. How can the taxman know that I transfered the coins from coinbase to binance?

provinzio commented 3 years ago

It doesn't. And it doesn't matter from where the coins come. It's just matters when you bought and sold.

And currently the history are given is missing some buys.

jhoogstraat commented 3 years ago

Well, I don't know how that could have happened. Gonna generate a new statement from binance..

provinzio commented 3 years ago

Well, if your history is only partially it's possible that you are missing some buys. E.g. you bought 1 btc on coinbase. Transfered 1 btc to binance and sold 1 btc in binance. If you only add the binance statement CoinTaxman will raise an error, because it's missing the information about the buy operation.

jhoogstraat commented 3 years ago

Yeah the statements have to be coherent. I am pretty sure, the statement is complete and does not miss any buys or sells. I did some investigation and the corresponding operation looks like this: bop.op.change = 0.01028688 bop.sold = 0.01028688 not_sold = 0E-8

Is 0E-8 expected here? Kinda seems like the two issues are connected, no?

Edit: Allowing 0E-8 fixes the issue, but I am not sure, if this is the right solution. As for the missing buy/sell op, CoinTaxman did not read the CoinbasePro statement. I left a fix at #31.

provinzio commented 3 years ago

Would you mind to test your account statment with ne newest commit again? I might have fixed your problem.

jhoogstraat commented 3 years ago

As you did not modify book.py in your newest commit, I guess assert change should still trigger (and it does). Removing these "0E-8" entries, the code runs fine.

Sadly the results are not usable, as many prices are still missing (not provided by binance for some reason..)

provinzio commented 3 years ago

Ah yeah, nvm. I thought of something different.

From when are the prices? If they are not older than 3 years would you mind sharing the coin type and the date?

jhoogstraat commented 3 years ago

Here are some examples:

https://api.binance.com/api/v3/aggTrades?symbol=ROSEBTC&startTime=1617870231000&endTime=1617870291000 https://api.binance.com/api/v3/aggTrades?symbol=MDTBTC&startTime=1617955766000&endTime=1617955826000 https://api.binance.com/api/v3/aggTrades?symbol=BTSBTC&startTime=1616737067000&endTime=1616737127000 https://api.binance.com/api/v3/aggTrades?symbol=ARPABTC&startTime=1617750754000&endTime=1617750814000 https://api.binance.com/api/v3/aggTrades?symbol=DENTBTC&startTime=1618057819000&endTime=1618057879000 https://api.binance.com/api/v3/aggTrades?symbol=QTUMBTC&startTime=1618154304000&endTime=1618154364000 https://api.binance.com/api/v3/aggTrades?symbol=LUNAEUR&startTime=1618938629000&endTime=1618938689000 https://api.binance.com/api/v3/aggTrades?symbol=UNIEUR&startTime=1618923446000&endTime=1618923506000

provinzio commented 2 years ago

Is this still an issue? Could have been solved by changing the saved prices from float to string.

jhoogstraat commented 2 years ago

I am positive that the issue was fixed, but I'll test using my statements later.