Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resolve deposits and withdrawls #4

Closed
2 tasks done
provinzio opened this issue Feb 18, 2021 · 17 comments · Fixed by #127
Closed
2 tasks done

Resolve deposits and withdrawls #4

provinzio opened this issue Feb 18, 2021 · 17 comments · Fixed by #127
Labels
Book good first issue Good for newcomers

Comments

@provinzio
Copy link
Owner

provinzio commented Feb 18, 2021

Foreign currency/coin deposits have to match withdrawls from another exchange. Otherwhise the program is not able to calculate the tax gains properly.

Furthermore we have to resolve deposits and withdrawals between the exchanges to correctly evaluate the taxation (#86).

We should check this in between the book read-in and the evaluation nd raise a warning or an exception.

  • Find matching withdrawals and deposits or raise a warning
  • Resolve withdrawals/deposits from one exchange to another with FIFO/LIFO principle (according to config): Move transactions from the withdrawal-exchange to the deposit-exchange by changing operations.platform. Transactions might have to be splitted for this.

Adressed in https://github.com/provinzio/CoinTaxman/tree/4-resolve-deposits

@provinzio provinzio added the good first issue Good for newcomers label Mar 2, 2021
@provinzio provinzio changed the title Print a warning for unmatched deposits/withdrawls Resolve deposits and withdrawls Nov 28, 2021
@provinzio provinzio added the Book label Nov 28, 2021
@Griffsano
Copy link
Contributor

Just to confirm: Changing operations.platform would also maintain the price information for buying the coin, right?
Example: I buy a coin at exchange A for price 100. Then I transfer the coin to exchange B, where it has a price of 130 at the time of transfer. I sell the coin at exchange B for price 200. In this example, the taxable gain should be 100 and not 70 (minus of course the transfer/buy/sell fees).

@provinzio
Copy link
Owner Author

Changing the platform might resolve in a different price. The price of a coin at buy time is dynamically evaluated with the get_price function. It checks the price of that coin at the buy time on the exchange operations.platform. So changing the platform changes the origin where the buy price comes from.

We might have to add an additional variable, so that we can distinguish between the buy-platform and the current platform.

@Griffsano
Copy link
Contributor

I agree, makes sense to add a new variable for the buy-platform and split up buy operations if necessary.

Already an idea for how to match deposits/withdrawals if the amount is not equal? (If I remember correctly, some exchanges directly withdraw the deposit fee before crediting the amount. -> We could back-calculate the fee by subtracting the deposit amount from the withdrawal amount.)
But there's always room for uncertainty, especially since transfers can take a while.

For deposits/withdrawals that cannot be matched, we could still move the respective buy operation to platform "unknown", so that at least the rest of the FIFO/LIFO tax calculation is correct.

@provinzio
Copy link
Owner Author

provinzio commented Jan 20, 2022

If it's unclear, we have to rely on user input and issue a warning or a question to the user. We might want to create a withdrawl-deposit-match.csv in which you can write from where to where a coin was moved to explicitly define a withdrawl-match-pair.

Is the withdrawal/deposit fee relevant for taxation?

@Griffsano
Copy link
Contributor

Yes, I also thought about a file that stores matching deposits and withdrawals like you explained. Possibly by storing the platform, transaction IDs (if available), timestamps, and if the fees are relevant for taxation (anything else?).
It might be worth going for this file-based approach first, then we technically would not even need the auto-matching feature (would still be nice of course and less work for the user).

I would say that the fees are relevant for taxation, if the assets are used for trading (not for buying stuff with crypto).
One could argue that the deposits/withdrawals are necessary expenses in order to trade at a certain platform. For example, some platforms have high fees for credit card / SWIFT payments or do not even support fiat deposits.

@Griffsano
Copy link
Contributor

I think for MULTI_DEPOT we would need a nested loop for that: First, sort all withdrawals chronologically, and go through the list. For each withdrawal, go to the platform and do the balancing similar to the taxation. Only then we can split up the respective buy operations correctly and move them to the deposit platform.
It would be difficult to do it directly in the taxation operations for-loop, as for each deposit we would need to down-trace the respective buy actions for the transferred coins.
What do you think, any better idea?

@provinzio
Copy link
Owner Author

provinzio commented Feb 20, 2022

I would resolve/match the withdrawals and deposits after the book reading and before the tax evaluation.

Check that each withdrawal Operation has a deposit operation at the same timestamp or shortly after the withdrawal. Check that the transfered coin matches (fees might reduce the withdrawn/deposited amount.

If we find a matching withdrawal/deposit, move some coins to the other platform. Now the fun question, which coins should be moved? I think that we can not move staked coins. But what about the others FIFO oder LIFO? This might lead to different tax evaluations. -- I would go with FIFO just because everything uses FIFO

@Griffsano
Copy link
Contributor

I agree with that we can't move staked coins and that we should use FIFO, but maybe we can even make it variable based on BalanceFIFOQueue/BalanceLIFOQueue. But still, in order to decide which coins / operations get moved, I think we have to do the full balancing of buys/sells/etc.

@provinzio
Copy link
Owner Author

provinzio commented Feb 20, 2022

You do not need to balance all coins. Just mark them or change the variables information beginning with the first in time.

Operations need a new member bought_on_platform which saves the platform in which it was bought (to query the buy price at all times). The platform member saves the platform in which the coin is currently. If we want to move a coin to another platform, just change the platform variable. If we want to move only 1 of 2 coins. Split the operation object in two parts. One which gets moved and one which stays at the old platform.

To check if a coin is staked, we need to save a list of dates (staked_at and retrieved_at) so we can check whether the coin is staked at s specific point in time. If the retrieved_at value is missing, it is still staked. If we only stake a part of the coins of an operation, we have to split the operation in two.

So we do

  1. Read in book
  2. Check which coins are at which time staked
  3. Match withdrawal an deposits
  4. Evaluate taxation

What do you think?

@Griffsano
Copy link
Contributor

What if you deposit a coin, which you stake afterwards? I think you have to evaluate staking and withdrawals in the same loop.
But, as we discussed in #57, we could use the same approach for staking, basically moving the asset to <platform>-staked.
It might be worth doing the balancing before the taxation, also because it's independent of the country.

@provinzio
Copy link
Owner Author

provinzio commented Feb 20, 2022

True, we should match deposit and withdrals at the same time we evaluate the staking. Same is true for selling. So we have to do everything in the same loop.

Problem: our tax evaluation is currently per platform. We need to change that and evaluate all operations sorted by time.

@Griffsano
Copy link
Contributor

What about evaluating all operations sorted by time, but having a separate balance for each platform? So we would have multiple instances of BalanceQueue. And we could do this step before _evaluate_taxation_{country}.

@provinzio
Copy link
Owner Author

I like your idea. Run through all operations sorted by time. And resolve all deposits, withdrawals and note which coins were staked at which times.

Using a balance queue per platform seems like a good idea for this.

Afterwards we run over all operations sorted by time per platform, doing the tax evaluation.
Keeping the resolvement of deposits/... separated from the localized tax evaluation will make our life's easier in the future.

@jhoogstraat
Copy link
Contributor

jhoogstraat commented Mar 27, 2022

This is only affecting the calculation when MULTI_DEPOT = True, no?
Otherwise, the withdrawals and deposits between exchanges should cancel out each other (e.g., doesn't matter where coins are bought or sold).

In case of MULTI_DEPOT = True, should Coinbase and Coinbase Pro be handled as separate exchanges (which they currently are)? The problem is that transfers between the two are not included in the exports, which in turn makes it relevant to explicitly state transfers in a withdrawals_deposits file.

@provinzio
Copy link
Owner Author

Yeah. This is for Multidepots tax evaluation and in general to pre-check whether all necessary Account statements exist.

What ist the difference between Coinbase and Coinbase pro?

@jhoogstraat
Copy link
Contributor

Coinbase Pro is a product by Coinbase aimed to provide a more professional trading ui. You do have separate wallets however.

@provinzio
Copy link
Owner Author

Task if this issue is only to determine which deposit is linked to which withdrawal for all non config.FIAT coins.

It's not necessary to define, which specific coins gets moved by this. This is done on the tax evaluation.

So balancing is not necessary here.

@provinzio provinzio linked a pull request May 5, 2022 that will close this issue
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Book good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants