provinzio / CoinTaxman

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

DB Hotfix #106

Closed Griffsano closed 2 years ago

Griffsano commented 2 years ago

This PR does the following things:

provinzio commented 2 years ago

Could you resolve the merge conflicts with main?

Griffsano commented 2 years ago

I see we've both worked on the warnings for overwriting DB prices. I merged main and kept your E-16 threshold for overwriting the DB price. I thought about suppressing warnings if the error is below e.g. 1%, but I think going with a lower value for numerical errors (such as E-16) is better.

Griffsano commented 2 years ago

@provinzio Based on the recent discussions in #114, this PR is ready from my side. It doesn't resolve #114 though, as the problem lies in the price fetching for Binance and, as it turned out, has not so much to do with this PR.

provinzio commented 2 years ago

Ups I did it again. Fixing the same error you already managed in this PR. 🤦‍♂️ I merged your work. Good job!

Also refactored some more things beside your PR, but matching the DB Hotfix theme ;)

Why should we remove the price addition from coinbase/bitpanda import? Explicit is better than implicit. There might be another buy at the same time, which could make matching difficult. Looks foolproof to me, just adding it directly.

provinzio commented 2 years ago

Why should we remove the price addition from coinbase/bitpanda import? Explicit is better than implicit. There might be another buy at the same time, which could make matching difficult. Looks foolproof to me, just adding it directly.

To avoid getting this PR stale again, I reverted the said changes above and merged this. Let's discuss, wether we want to remove this or not. I am eager to hear your opinion.

Griffsano commented 2 years ago

Hey @provinzio, the problem with the explicit prices in the Coinbase CSVs is that they are rounded (seems like always two digits after the comma). Therefore, the calculated price based on the transactions is more accurate. This leads to overwriting of DB prices (at least the first time once the calculated prices are set), and a lot of warning entries since the prices do not match (on every run). However, we could use the same approach of writing in the database when reading in the file, only that we calculate the prices instead of using the field in the CSV.

provinzio commented 2 years ago

That makes sense.. sounds annoying. But I believe that it might be safer to keep adding the prices from csv. In case it's not possibly to query a transaction price from the API (e.g. historical prices or other weird reasons).

On another matter, I thought about adding a remark column to the price db. We could still add prices from csv and add the comment "from csv filexyz.xlsx line 123" or add more columns to make this easier addable. This additional info given, we could avoid the warning, wen the API coin price has more decimals. But should warn when the price differs by more than the last decimals.

What do you think about that?

Griffsano commented 2 years ago

That remark/source column for the DB sounds good, I think that point already came up in some other issue/PR. This would also give us the possibility to filter and delete entries from certain sources, if we find out later that there was an error in the price calculation. For Coinbase, we could stick with the approach of writing in the database when reading in the file, only that we calculate the prices based on buy/sell instead of using the price field from the CSV. What do you think about that?

provinzio commented 2 years ago

calculate the prices based on buy/sell

what do you mean by that. I don't get the difference. Feel free to create a PR with your proposal.