CoinTaxman icon indicating copy to clipboard operation
CoinTaxman copied to clipboard

Resolve ops in edge cases

Open jhoogstraat opened this issue 2 years ago • 6 comments

This is the pr for issue #135.

It's not finished yet, but I wanted to allow for feedback early on.

Implemented edge cases:

  • [x] Two pairs at a particular utc_time via a bridge coin
  • [ ] Orders not being fulfilled at once, leading to Buy/Sell ops later on (can this even happen? Buy/Sell come in pairs, no?)
  • [x] Commissions / Withdrawals / Deposits / Airdrop / CoinLendInterest not relevant for trade linking
  • [x] BNB small asset exchange at multiple utc_times (buy/sell 1 second apart for me)
  • [ ] Fees paid (partly) in BNB in the double_buy_sell_pair case

Currently, the is_binance_bnb_small_asset_transfer case catches almost everything that was not handled by the other cases. I tried my best to make sure that only ops that really are small asset exchanges get handled (assert that the buy coin is BNB).

jhoogstraat avatar May 16 '22 12:05 jhoogstraat

I merged the two methods for resolving fees and trades into one. This reduces duplication by quite a bit. It also enables us to resolve fees easily when there are two buy/sell-pairs at any given utc_time. I know that separating the two methods is good practice, but I think in this case it adds too much duplication and complexity.

I am not sure how to account for fees paid in bnb when there is more than one buy op... I added a BUG to the code at the relevant line. How can we detect which bnb-fee op belongs to which buy op?

jhoogstraat avatar May 16 '22 23:05 jhoogstraat

I merged the two methods for resolving fees and trades into one.

Make sure that resolving fees and resolving trades can be something different. E.g. the fee could also belong to a deposit

I am not sure how to account for fees paid in bnb when there is more than one buy op... I added a BUG to the code at the relevant line. How can we detect which bnb-fee op belongs to which buy op?

Split the BNB fee according to the "value" of the sell. We can use the price in eur to estimate that

provinzio avatar May 17 '22 05:05 provinzio

Make sure that resolving fees and resolving trades can be something different. E.g. the fee could also belong to a deposit

True. Maybe have fees done in multiple passes? What do you prefer / think is better?

Split the BNB fee according to the "value" of the sell. We can use the price in eur to estimate that

Like calculating the expected fee and find a combination of fee ops matches the calculated fee best? Fees can be quite different depending on volume on Binance. Don't know about other platforms.

jhoogstraat avatar May 17 '22 06:05 jhoogstraat

True. Maybe have fees done in multiple passes? What do you prefer / think is better?

Matching of deposit/withdrawals, coin lend/staking start and end, trading pairs and finally matching everything with fees should be possible in the same loop

In that case we might have to group and match everything before the "merge of equal operations"

provinzio avatar May 17 '22 06:05 provinzio

Im am first going to concentrate on trades with fees. We can add lending/staking, etc. later.

jhoogstraat avatar May 18 '22 09:05 jhoogstraat

Hey @jhoogstraat,

I went through the code and did some formatting. Didn't wanted to bother you with that. I merged the newest main into the branch, fixed the linting errors and added some more assert checks.

Changes look good to me. I'd love when you could proof read my commits. Is the PR ready to be merged? Looking forward to your reply. For now, I'll mark this as draft to keep an overview. :)

provinzio avatar Jun 04 '22 08:06 provinzio