From 632167f5d6046f0e7385eeaa29eb2a029ed5585c Mon Sep 17 00:00:00 2001 From: Joshua Hoogstraat <34964599+jhoogstraat@users.noreply.github.com> Date: Mon, 16 May 2022 14:00:26 +0200 Subject: [PATCH 01/14] Add trade resolving when there are two pairs --- src/book.py | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 54 insertions(+), 3 deletions(-) diff --git a/src/book.py b/src/book.py index dd3b535..1cbc00d 100644 --- a/src/book.py +++ b/src/book.py @@ -21,7 +21,7 @@ import re from collections import defaultdict from pathlib import Path -from typing import Any, Optional +from typing import Any, Counter, Optional import config import log_config @@ -1582,14 +1582,25 @@ def match_fees(self) -> None: ) def resolve_trades(self) -> None: + # Filter operations to only contain trades. + filtered_ops = [ + op + for op in self.operations + if op.type_name + in [ + tr.Buy.type_name_c(), + tr.Sell.type_name_c(), + ] + ] + # Match trades which belong together (traded at same time). - for _, _operations in misc.group_by(self.operations, "platform").items(): + for _, _operations in misc.group_by(filtered_ops, "platform").items(): for _, matching_operations in misc.group_by( _operations, "utc_time" ).items(): # Count matching operations by type with dict # { operation typename: list of operations } - t_op = collections.defaultdict(list) + t_op = collections.defaultdict(list[tr.Operation]) for op in matching_operations: t_op[op.type_name].append(op) @@ -1653,6 +1664,46 @@ def resolve_trades(self) -> None: ) continue + # Double buy/sell-pairs via a "bridge" coin at a + # particular utc_time (e.g. sell btc/usdt and buy eth/usdt) + # Find the coin that has one buy and one sell op + bridge_coin = Counter( + [op.coin for op in t_op[tr.Buy.type_name_c()]] + ) & Counter([op.coin for op in t_op[tr.Sell.type_name_c()]]) + + is_double_buy_sell_pair = all( + ( + len(matching_operations) == 4, + len(t_op[tr.Buy.type_name_c()]) == 2, + len(t_op[tr.Sell.type_name_c()]) == 2, + len(bridge_coin) == 1, + ) + ) + if is_double_buy_sell_pair: + # Get the name of the bridge coin + bridge_coin_name = next(bridge_coin.elements()) + + # Iterate over all ops and add links accordingly + for buy_op in t_op[tr.Buy.type_name_c()]: + assert isinstance(buy_op, tr.Buy) + assert buy_op.link is buy_op.buying_cost is None + for sell_op in t_op[tr.Sell.type_name_c()]: + assert isinstance(sell_op, tr.Sell) + assert sell_op.selling_value is None + if sell_op.link is not None: + continue + is_valid_pair = [buy_op.coin, sell_op.coin].count( + bridge_coin_name + ) == 1 + if is_valid_pair: + buy_op.link = sell_op + sell_op.link = buy_op + else: + assert True, "This should not happen" + continue + + log.warning(f"Failed: {matching_operations}") + def read_file(self, file_path: Path) -> None: """Import transactions form an account statement. From d4e51786cf89005b0385f9195e81b8dc6d950629 Mon Sep 17 00:00:00 2001 From: Joshua Hoogstraat <34964599+jhoogstraat@users.noreply.github.com> Date: Tue, 17 May 2022 01:25:08 +0200 Subject: [PATCH 02/14] Merge resolving trades and fees --- src/book.py | 119 ++++++++++++++++------------------------------------ src/main.py | 3 +- 2 files changed, 38 insertions(+), 84 deletions(-) diff --git a/src/book.py b/src/book.py index 1cbc00d..47ce3f0 100644 --- a/src/book.py +++ b/src/book.py @@ -1518,82 +1518,16 @@ def merge_identical_operations(self) -> None: grouped_ops = misc.group_by(self.operations, tr.Operation.identical_columns) self.operations = [tr.Operation.merge(*ops) for ops in grouped_ops.values()] - def match_fees(self) -> None: - # Split operations in fees and other operations. - operations = [] - all_fees: list[tr.Fee] = [] - - for op in self.operations: - if isinstance(op, tr.Fee): - all_fees.append(op) - else: - operations.append(op) - - # Only keep none fee operations in book. - self.operations = operations - - # Match fees to book operations. - for platform, _fees in misc.group_by(all_fees, "platform").items(): - for utc_time, fees in misc.group_by(_fees, "utc_time").items(): - - # Find matching operations by platform and time. - matching_operations = { - idx: op - for idx, op in enumerate(self.operations) - if op.platform == platform and op.utc_time == utc_time - } - - # Group matching operations in dict with - # { operation typename: list of indices } - t_op = collections.defaultdict(list) - for idx, op in matching_operations.items(): - t_op[op.type_name].append(idx) - - # Check if this is a buy/sell-pair. - # Fees might occure by other operation types, - # but this is currently not implemented. - is_buy_sell_pair = all( - ( - len(matching_operations) == 2, - len(t_op[tr.Buy.type_name_c()]) == 1, - len(t_op[tr.Sell.type_name_c()]) == 1, - ) - ) - if is_buy_sell_pair: - # Fees have to be added to all buys and sells. - # 1. Fees on sells are the transaction cost, - # which might be fully tax relevant for this sell - # and which gets removed from the account balance - # 2. Fees on buys increase the buy-in price of the coins - # which is relevant when selling these (not buying) - (sell_idx,) = t_op[tr.Sell.type_name_c()] - (buy_idx,) = t_op[tr.Buy.type_name_c()] - assert self.operations[sell_idx].fees is None - assert self.operations[buy_idx].fees is None - self.operations[sell_idx].fees = fees - self.operations[buy_idx].fees = fees - else: - log.warning( - "Fee matching is not implemented for this case. " - "Your fees will be discarded and are not evaluated in " - "the tax evaluation.\n" - "Please create an Issue or PR.\n\n" - f"{matching_operations=}\n{fees=}" - ) - - def resolve_trades(self) -> None: - # Filter operations to only contain trades. + def resolve_trades_and_fees(self) -> None: + # Filter operations to only contain trades and fees. filtered_ops = [ op for op in self.operations if op.type_name - in [ - tr.Buy.type_name_c(), - tr.Sell.type_name_c(), - ] + in [tr.Buy.type_name_c(), tr.Sell.type_name_c(), tr.Fee.type_name_c()] ] - # Match trades which belong together (traded at same time). + # Match trades which belong together (traded at same time) and add belonging fees. for _, _operations in misc.group_by(filtered_ops, "platform").items(): for _, matching_operations in misc.group_by( _operations, "utc_time" @@ -1604,14 +1538,14 @@ def resolve_trades(self) -> None: for op in matching_operations: t_op[op.type_name].append(op) - # Check if this is a buy/sell-pair. + # Check if this is a single buy/sell-pair. # Fees might occure by other operation types, # but this is currently not implemented. is_buy_sell_pair = all( ( - len(matching_operations) == 2, len(t_op[tr.Buy.type_name_c()]) == 1, len(t_op[tr.Sell.type_name_c()]) == 1, + len(t_op[tr.Fee.type_name_c()]) <= 2, ) ) if is_buy_sell_pair: @@ -1620,12 +1554,17 @@ def resolve_trades(self) -> None: assert isinstance(buy_op, tr.Buy) (sell_op,) = t_op[tr.Sell.type_name_c()] assert isinstance(sell_op, tr.Sell) + fees = t_op[tr.Fee.type_name_c()] assert buy_op.link is None assert buy_op.buying_cost is None buy_op.link = sell_op assert sell_op.link is None assert sell_op.selling_value is None sell_op.link = buy_op + assert buy_op.fees is None + buy_op.fees = fees + assert sell_op.fees is None + sell_op.fees = fees continue # Binance allows to convert small assets in one go to BNB. @@ -1640,7 +1579,6 @@ def resolve_trades(self) -> None: all(op.platform == "binance" for op in matching_operations), len(t_op[tr.Buy.type_name_c()]) == 1, len(t_op[tr.Sell.type_name_c()]) >= 1, - len(t_op.keys()) == 2, ) ) @@ -1666,22 +1604,22 @@ def resolve_trades(self) -> None: # Double buy/sell-pairs via a "bridge" coin at a # particular utc_time (e.g. sell btc/usdt and buy eth/usdt) - # Find the coin that has one buy and one sell op - bridge_coin = Counter( - [op.coin for op in t_op[tr.Buy.type_name_c()]] - ) & Counter([op.coin for op in t_op[tr.Sell.type_name_c()]]) + # Find the coin that has one buy op and one sell op + buy_coins = set([op.coin for op in t_op[tr.Buy.type_name_c()]]) + sell_coins = set([op.coin for op in t_op[tr.Sell.type_name_c()]]) + bridge_coins = buy_coins & sell_coins is_double_buy_sell_pair = all( ( - len(matching_operations) == 4, len(t_op[tr.Buy.type_name_c()]) == 2, len(t_op[tr.Sell.type_name_c()]) == 2, - len(bridge_coin) == 1, + len(t_op[tr.Fee.type_name_c()]) <= 3, + len(bridge_coins) == 1, ) ) if is_double_buy_sell_pair: # Get the name of the bridge coin - bridge_coin_name = next(bridge_coin.elements()) + bridge_coin = bridge_coins.pop() # Iterate over all ops and add links accordingly for buy_op in t_op[tr.Buy.type_name_c()]: @@ -1693,16 +1631,33 @@ def resolve_trades(self) -> None: if sell_op.link is not None: continue is_valid_pair = [buy_op.coin, sell_op.coin].count( - bridge_coin_name + bridge_coin ) == 1 if is_valid_pair: buy_op.link = sell_op sell_op.link = buy_op + # Match fees to buy_op coin + # BUG Fees paid in BNB are currently ignored. + # Is is unclear which buy op the BNB-fee op should be accounted to. + # It is also possible to pay fees partly in the coin bought and partly in BNB + fees = [ + fee + for fee in t_op[tr.Fee.type_name_c()] + if fee.coin == buy_op.coin + ] + assert len(fees) == 1 + buy_op.fees = fees + sell_op.fees = fees else: assert True, "This should not happen" continue - log.warning(f"Failed: {matching_operations}") + log.warning(f"Matching trades failed ops={t_op}") + + # Only keep none fee operations in book. + self.operations = [ + op for op in self.operations if op.type_name != tr.Fee.type_name_c() + ] def read_file(self, file_path: Path) -> None: """Import transactions form an account statement. diff --git a/src/main.py b/src/main.py index 4de1a62..2fcb70f 100644 --- a/src/main.py +++ b/src/main.py @@ -51,8 +51,7 @@ def main() -> None: # Match fees with operations AND # Resolve dependencies between sells and buys, which is # necessary to correctly calculate the buying cost of a sold coin - book.match_fees() - book.resolve_trades() + book.resolve_trades_and_fees() taxman.evaluate_taxation() evaluation_file_path = taxman.export_evaluation_as_excel() From 915f16fbc23e2b404b736ce37d77189e0ead890d Mon Sep 17 00:00:00 2001 From: Joshua Hoogstraat <34964599+jhoogstraat@users.noreply.github.com> Date: Tue, 17 May 2022 09:02:45 +0200 Subject: [PATCH 03/14] rearrange cases, handle bnb small asset edge case --- src/book.py | 77 ++++++++++++++++++++++++++++------------------------- 1 file changed, 41 insertions(+), 36 deletions(-) diff --git a/src/book.py b/src/book.py index 47ce3f0..02be991 100644 --- a/src/book.py +++ b/src/book.py @@ -1528,6 +1528,7 @@ def resolve_trades_and_fees(self) -> None: ] # Match trades which belong together (traded at same time) and add belonging fees. + bnb_small_asset_sell_cache: list[tr.Sell] = [] for _, _operations in misc.group_by(filtered_ops, "platform").items(): for _, matching_operations in misc.group_by( _operations, "utc_time" @@ -1567,41 +1568,6 @@ def resolve_trades_and_fees(self) -> None: sell_op.fees = fees continue - # Binance allows to convert small assets in one go to BNB. - # Our `merge_identical_column` function merges all BNB which - # gets bought at that time together. - # BUG Trade connection can not be established with our current - # method. - # Calculate the buying cost of this type of operation by all - # small asset sells. - is_binance_bnb_small_asset_transfer = all( - ( - all(op.platform == "binance" for op in matching_operations), - len(t_op[tr.Buy.type_name_c()]) == 1, - len(t_op[tr.Sell.type_name_c()]) >= 1, - ) - ) - - if is_binance_bnb_small_asset_transfer: - (buy_op,) = t_op[tr.Buy.type_name_c()] - assert isinstance(buy_op, tr.Buy) - sell_ops = t_op[tr.Sell.type_name_c()] - assert all(isinstance(op, tr.Sell) for op in sell_ops) - assert buy_op.link is None - assert buy_op.buying_cost is None - buying_costs = [self.price_data.get_cost(op) for op in sell_ops] - buy_op.buying_cost = misc.dsum(buying_costs) - assert len(sell_ops) == len(buying_costs) - for sell_op, buying_cost in zip(sell_ops, buying_costs): - assert isinstance(sell_op, tr.Sell) - assert sell_op.link is None - assert sell_op.selling_value is None - percent = buying_cost / buy_op.buying_cost - sell_op.selling_value = self.price_data.get_partial_cost( - buy_op, percent - ) - continue - # Double buy/sell-pairs via a "bridge" coin at a # particular utc_time (e.g. sell btc/usdt and buy eth/usdt) # Find the coin that has one buy op and one sell op @@ -1613,7 +1579,7 @@ def resolve_trades_and_fees(self) -> None: ( len(t_op[tr.Buy.type_name_c()]) == 2, len(t_op[tr.Sell.type_name_c()]) == 2, - len(t_op[tr.Fee.type_name_c()]) <= 3, + 0 < len(t_op[tr.Fee.type_name_c()]) <= 3, len(bridge_coins) == 1, ) ) @@ -1652,6 +1618,45 @@ def resolve_trades_and_fees(self) -> None: assert True, "This should not happen" continue + # Binance allows to convert small assets in one go to BNB. + # Our `merge_identical_column` function merges all BNB which + # gets bought at that time together. + # BUG Trade connection can not be established with our current + # method. + # Calculate the buying cost of this type of operation by all + # small asset sells. + # This method relies on the operations being sorted by utc_time. + is_binance_bnb_small_asset_transfer = all(op.platform == "binance" for op in matching_operations), + + if is_binance_bnb_small_asset_transfer: + if len(t_op[tr.Buy.type_name_c()]) == 0: + bnb_small_asset_sell_cache += t_op[tr.Sell.type_name_c()] + continue + else: + (buy_op,) = t_op[tr.Buy.type_name_c()] + assert isinstance(buy_op, tr.Buy) + sell_ops = ( + bnb_small_asset_sell_cache + t_op[tr.Sell.type_name_c()] + ) + assert len(sell_ops) > 0 + assert all(isinstance(op, tr.Sell) for op in sell_ops) + assert buy_op.link is None + assert buy_op.buying_cost is None + buying_costs = [self.price_data.get_cost(op) for op in sell_ops] + buy_op.buying_cost = misc.dsum(buying_costs) + assert len(sell_ops) == len(buying_costs) + for sell_op, buying_cost in zip(sell_ops, buying_costs): + assert isinstance(sell_op, tr.Sell) + assert sell_op.link is None + assert sell_op.selling_value is None + percent = buying_cost / buy_op.buying_cost + sell_op.selling_value = self.price_data.get_partial_cost( + buy_op, percent + ) + # Clear cache after all sell ops are accounted for. + bnb_small_asset_sell_cache.clear() + continue + log.warning(f"Matching trades failed ops={t_op}") # Only keep none fee operations in book. From caf2f5c7aa6cfeffa25b5e9e58fbc01877bb9fb2 Mon Sep 17 00:00:00 2001 From: Joshua Hoogstraat <34964599+jhoogstraat@users.noreply.github.com> Date: Tue, 17 May 2022 09:10:09 +0200 Subject: [PATCH 04/14] formatting --- src/book.py | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/book.py b/src/book.py index 02be991..3689396 100644 --- a/src/book.py +++ b/src/book.py @@ -1527,8 +1527,10 @@ def resolve_trades_and_fees(self) -> None: in [tr.Buy.type_name_c(), tr.Sell.type_name_c(), tr.Fee.type_name_c()] ] - # Match trades which belong together (traded at same time) and add belonging fees. + # Cache for sell ops if the belonging buy op happens at a later utc_time bnb_small_asset_sell_cache: list[tr.Sell] = [] + + # Match trades which belong together (traded at same time) and add belonging fees. for _, _operations in misc.group_by(filtered_ops, "platform").items(): for _, matching_operations in misc.group_by( _operations, "utc_time" @@ -1568,9 +1570,10 @@ def resolve_trades_and_fees(self) -> None: sell_op.fees = fees continue - # Double buy/sell-pairs via a "bridge" coin at a - # particular utc_time (e.g. sell btc/usdt and buy eth/usdt) - # Find the coin that has one buy op and one sell op + # Double buy/sell-pairs via a "bridge" coin + # e.g. sell btc/usdt and buy eth/usdt (usdt as a bridge coin) + + # Find the bridge coin which has one buy op and one sell op buy_coins = set([op.coin for op in t_op[tr.Buy.type_name_c()]]) sell_coins = set([op.coin for op in t_op[tr.Sell.type_name_c()]]) bridge_coins = buy_coins & sell_coins @@ -1626,12 +1629,13 @@ def resolve_trades_and_fees(self) -> None: # Calculate the buying cost of this type of operation by all # small asset sells. # This method relies on the operations being sorted by utc_time. - is_binance_bnb_small_asset_transfer = all(op.platform == "binance" for op in matching_operations), + is_binance_bnb_small_asset_transfer = all( + op.platform == "binance" for op in matching_operations + ) if is_binance_bnb_small_asset_transfer: if len(t_op[tr.Buy.type_name_c()]) == 0: bnb_small_asset_sell_cache += t_op[tr.Sell.type_name_c()] - continue else: (buy_op,) = t_op[tr.Buy.type_name_c()] assert isinstance(buy_op, tr.Buy) @@ -1655,7 +1659,7 @@ def resolve_trades_and_fees(self) -> None: ) # Clear cache after all sell ops are accounted for. bnb_small_asset_sell_cache.clear() - continue + continue log.warning(f"Matching trades failed ops={t_op}") From ca66e4145cf4b3483ec58a91601a1a9a647ce760 Mon Sep 17 00:00:00 2001 From: Joshua Hoogstraat <34964599+jhoogstraat@users.noreply.github.com> Date: Tue, 17 May 2022 11:36:31 +0200 Subject: [PATCH 05/14] Warning on BNB fees and formatting --- src/book.py | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/src/book.py b/src/book.py index 3689396..91798e0 100644 --- a/src/book.py +++ b/src/book.py @@ -1521,10 +1521,7 @@ def merge_identical_operations(self) -> None: def resolve_trades_and_fees(self) -> None: # Filter operations to only contain trades and fees. filtered_ops = [ - op - for op in self.operations - if op.type_name - in [tr.Buy.type_name_c(), tr.Sell.type_name_c(), tr.Fee.type_name_c()] + op for op in self.operations if isinstance(op, (tr.Buy, tr.Sell, tr.Fee)) ] # Cache for sell ops if the belonging buy op happens at a later utc_time @@ -1548,7 +1545,7 @@ def resolve_trades_and_fees(self) -> None: ( len(t_op[tr.Buy.type_name_c()]) == 1, len(t_op[tr.Sell.type_name_c()]) == 1, - len(t_op[tr.Fee.type_name_c()]) <= 2, + 0 < len(t_op[tr.Fee.type_name_c()]) <= 2, # coin + bnb fee ) ) if is_buy_sell_pair: @@ -1577,12 +1574,11 @@ def resolve_trades_and_fees(self) -> None: buy_coins = set([op.coin for op in t_op[tr.Buy.type_name_c()]]) sell_coins = set([op.coin for op in t_op[tr.Sell.type_name_c()]]) bridge_coins = buy_coins & sell_coins - is_double_buy_sell_pair = all( ( len(t_op[tr.Buy.type_name_c()]) == 2, len(t_op[tr.Sell.type_name_c()]) == 2, - 0 < len(t_op[tr.Fee.type_name_c()]) <= 3, + 0 < len(t_op[tr.Fee.type_name_c()]) <= 2, # coin + bnb fee len(bridge_coins) == 1, ) ) @@ -1619,6 +1615,10 @@ def resolve_trades_and_fees(self) -> None: sell_op.fees = fees else: assert True, "This should not happen" + if any(op.coin == "BNB" for op in t_op[tr.Fee.type_name_c()]): + log.debug( + f"Fees paid in BNB not considered. Please open an issue or PR" + ) continue # Binance allows to convert small assets in one go to BNB. @@ -1630,7 +1630,10 @@ def resolve_trades_and_fees(self) -> None: # small asset sells. # This method relies on the operations being sorted by utc_time. is_binance_bnb_small_asset_transfer = all( - op.platform == "binance" for op in matching_operations + ( + all(op.platform == "binance" for op in matching_operations), + len(t_op[tr.Buy.type_name_c()]) <= 1, + ) ) if is_binance_bnb_small_asset_transfer: From 66d4b22c52cdb2636f707b024f88d87887ba6f31 Mon Sep 17 00:00:00 2001 From: Joshua Hoogstraat <34964599+jhoogstraat@users.noreply.github.com> Date: Tue, 17 May 2022 12:36:55 +0200 Subject: [PATCH 06/14] warning instead of debug log --- src/book.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/book.py b/src/book.py index 91798e0..12ce551 100644 --- a/src/book.py +++ b/src/book.py @@ -1616,7 +1616,7 @@ def resolve_trades_and_fees(self) -> None: else: assert True, "This should not happen" if any(op.coin == "BNB" for op in t_op[tr.Fee.type_name_c()]): - log.debug( + log.warning( f"Fees paid in BNB not considered. Please open an issue or PR" ) continue From db1882619f020fad5550da4623f9a83997c5092b Mon Sep 17 00:00:00 2001 From: Joshua Hoogstraat <34964599+jhoogstraat@users.noreply.github.com> Date: Wed, 18 May 2022 13:38:06 +0200 Subject: [PATCH 07/14] bugfixes --- src/book.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/book.py b/src/book.py index 12ce551..81302d4 100644 --- a/src/book.py +++ b/src/book.py @@ -1528,7 +1528,7 @@ def resolve_trades_and_fees(self) -> None: bnb_small_asset_sell_cache: list[tr.Sell] = [] # Match trades which belong together (traded at same time) and add belonging fees. - for _, _operations in misc.group_by(filtered_ops, "platform").items(): + for platform, _operations in misc.group_by(filtered_ops, "platform").items(): for _, matching_operations in misc.group_by( _operations, "utc_time" ).items(): @@ -1545,7 +1545,7 @@ def resolve_trades_and_fees(self) -> None: ( len(t_op[tr.Buy.type_name_c()]) == 1, len(t_op[tr.Sell.type_name_c()]) == 1, - 0 < len(t_op[tr.Fee.type_name_c()]) <= 2, # coin + bnb fee + len(t_op[tr.Fee.type_name_c()]) <= 2, # coin + bnb fee ) ) if is_buy_sell_pair: @@ -1631,7 +1631,7 @@ def resolve_trades_and_fees(self) -> None: # This method relies on the operations being sorted by utc_time. is_binance_bnb_small_asset_transfer = all( ( - all(op.platform == "binance" for op in matching_operations), + platform == "binance", len(t_op[tr.Buy.type_name_c()]) <= 1, ) ) @@ -1642,6 +1642,7 @@ def resolve_trades_and_fees(self) -> None: else: (buy_op,) = t_op[tr.Buy.type_name_c()] assert isinstance(buy_op, tr.Buy) + assert buy_op.coin == "BNB" sell_ops = ( bnb_small_asset_sell_cache + t_op[tr.Sell.type_name_c()] ) From 743c42c792a853827737c0de20df1b9b474e6079 Mon Sep 17 00:00:00 2001 From: Jeppy Date: Sat, 4 Jun 2022 08:35:13 +0200 Subject: [PATCH 08/14] RM unused module typing.Counter --- src/book.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/book.py b/src/book.py index 81302d4..a9f51fe 100644 --- a/src/book.py +++ b/src/book.py @@ -21,7 +21,7 @@ import re from collections import defaultdict from pathlib import Path -from typing import Any, Counter, Optional +from typing import Any, Optional import config import log_config From 4099310fba50d9440dac8be35a642421613e0138 Mon Sep 17 00:00:00 2001 From: Jeppy Date: Sat, 4 Jun 2022 09:39:59 +0200 Subject: [PATCH 09/14] FIX mypy errors and UPDATE format and comments --- src/book.py | 105 ++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 77 insertions(+), 28 deletions(-) diff --git a/src/book.py b/src/book.py index a9f51fe..25a298d 100644 --- a/src/book.py +++ b/src/book.py @@ -1519,12 +1519,12 @@ def merge_identical_operations(self) -> None: self.operations = [tr.Operation.merge(*ops) for ops in grouped_ops.values()] def resolve_trades_and_fees(self) -> None: - # Filter operations to only contain trades and fees. + # Only look at trades and fees. filtered_ops = [ op for op in self.operations if isinstance(op, (tr.Buy, tr.Sell, tr.Fee)) ] - # Cache for sell ops if the belonging buy op happens at a later utc_time + # Cache for sell ops if the belonging buy op happens at a later utc_time. bnb_small_asset_sell_cache: list[tr.Sell] = [] # Match trades which belong together (traded at same time) and add belonging fees. @@ -1532,15 +1532,13 @@ def resolve_trades_and_fees(self) -> None: for _, matching_operations in misc.group_by( _operations, "utc_time" ).items(): - # Count matching operations by type with dict + # Count matching operations by type with dict. # { operation typename: list of operations } - t_op = collections.defaultdict(list[tr.Operation]) + t_op: dict[str, list[tr.Operation]] = collections.defaultdict(list) for op in matching_operations: t_op[op.type_name].append(op) # Check if this is a single buy/sell-pair. - # Fees might occure by other operation types, - # but this is currently not implemented. is_buy_sell_pair = all( ( len(t_op[tr.Buy.type_name_c()]) == 1, @@ -1554,7 +1552,11 @@ def resolve_trades_and_fees(self) -> None: assert isinstance(buy_op, tr.Buy) (sell_op,) = t_op[tr.Sell.type_name_c()] assert isinstance(sell_op, tr.Sell) - fees = t_op[tr.Fee.type_name_c()] + fees: list[tr.Fee] = t_op[ + tr.Fee.type_name_c() + ] # type:ignore[assignment] + assert all(isinstance(fee, tr.Fee) for fee in fees) + assert buy_op.link is None assert buy_op.buying_cost is None buy_op.link = sell_op @@ -1565,12 +1567,13 @@ def resolve_trades_and_fees(self) -> None: buy_op.fees = fees assert sell_op.fees is None sell_op.fees = fees + continue # Double buy/sell-pairs via a "bridge" coin - # e.g. sell btc/usdt and buy eth/usdt (usdt as a bridge coin) + # e.g. sell btc/usdt and buy eth/usdt (usdt as a bridge coin). - # Find the bridge coin which has one buy op and one sell op + # Find the bridge coin which has one buy op and one sell op. buy_coins = set([op.coin for op in t_op[tr.Buy.type_name_c()]]) sell_coins = set([op.coin for op in t_op[tr.Sell.type_name_c()]]) bridge_coins = buy_coins & sell_coins @@ -1578,7 +1581,7 @@ def resolve_trades_and_fees(self) -> None: ( len(t_op[tr.Buy.type_name_c()]) == 2, len(t_op[tr.Sell.type_name_c()]) == 2, - 0 < len(t_op[tr.Fee.type_name_c()]) <= 2, # coin + bnb fee + len(t_op[tr.Fee.type_name_c()]) <= 2, # coin + bnb fee len(bridge_coins) == 1, ) ) @@ -1589,40 +1592,63 @@ def resolve_trades_and_fees(self) -> None: # Iterate over all ops and add links accordingly for buy_op in t_op[tr.Buy.type_name_c()]: assert isinstance(buy_op, tr.Buy) - assert buy_op.link is buy_op.buying_cost is None + assert buy_op.link is None + for sell_op in t_op[tr.Sell.type_name_c()]: assert isinstance(sell_op, tr.Sell) - assert sell_op.selling_value is None + if sell_op.link is not None: + # Already matched. continue + is_valid_pair = [buy_op.coin, sell_op.coin].count( bridge_coin ) == 1 if is_valid_pair: + assert sell_op.link is not None buy_op.link = sell_op sell_op.link = buy_op # Match fees to buy_op coin # BUG Fees paid in BNB are currently ignored. - # Is is unclear which buy op the BNB-fee op should be accounted to. + # It is unclear which buy op the BNB-fee op should be accounted to. # It is also possible to pay fees partly in the coin bought and partly in BNB fees = [ - fee + fee # type:ignore[misc] for fee in t_op[tr.Fee.type_name_c()] - if fee.coin == buy_op.coin + if fee.coin in [buy_op.coin, sell_op.coin] ] - assert len(fees) == 1 + assert all(isinstance(fee, tr.Fee) for fee in fees) + assert len(fees) <= 1 buy_op.fees = fees sell_op.fees = fees else: - assert True, "This should not happen" - if any(op.coin == "BNB" for op in t_op[tr.Fee.type_name_c()]): + raise RuntimeError("Unexpected behavior") + + assert all( + op.link is not None + for op in t_op.values() + if isinstance(op, (tr.Buy, tr.Sell)) + ), "Not all operations were matched." + + linked_fees = set( + op.fees + for op in t_op[tr.Buy.type_name_c()] + + t_op[tr.Sell.type_name_c()] + ) + missing_fees = [ + fee + for fee in t_op[tr.Fee.type_name_c()] + if fee not in linked_fees + ] + if any(missing_fees): log.warning( - f"Fees paid in BNB not considered. Please open an issue or PR" + f"Following fees were not considered: {', '.join(f.coin for f in fees)}. " + "Please open an issue or PR." ) continue # Binance allows to convert small assets in one go to BNB. - # Our `merge_identical_column` function merges all BNB which + # Our `merge_identical_operations` function merges all BNB which # gets bought at that time together. # BUG Trade connection can not be established with our current # method. @@ -1638,20 +1664,45 @@ def resolve_trades_and_fees(self) -> None: if is_binance_bnb_small_asset_transfer: if len(t_op[tr.Buy.type_name_c()]) == 0: - bnb_small_asset_sell_cache += t_op[tr.Sell.type_name_c()] + # There are no buys. + # These sells might belong to a small asset transfer from another utc_time. + # Cache the sell operations. + sell_ops: list[tr.Sell] = t_op[ + tr.Sell.type_name_c() + ] # type:ignore[assignment] + assert all(isinstance(op, tr.Sell) for op in sell_ops) + + if t_op[tr.Fee.type_name_c()]: + raise NotImplementedError( + "Fees are currently not implemented for this scenario" + ) + + assert len(sell_ops) == len(t_op) + bnb_small_asset_sell_cache += sell_ops + else: (buy_op,) = t_op[tr.Buy.type_name_c()] assert isinstance(buy_op, tr.Buy) - assert buy_op.coin == "BNB" - sell_ops = ( - bnb_small_asset_sell_cache + t_op[tr.Sell.type_name_c()] - ) + assert ( + buy_op.coin == "BNB" + ), f"expected bnb small asset transfer, but coin is {buy_op.coin}" + sell_ops = t_op[ # type:ignore[assignment] + tr.Sell.type_name_c() + ] + + # Add previously cached sells to the list. + # TODO is it possible, that they occure after this timestamp + # check timestamps! + sell_ops += bnb_small_asset_sell_cache + assert len(sell_ops) > 0 assert all(isinstance(op, tr.Sell) for op in sell_ops) + assert buy_op.link is None assert buy_op.buying_cost is None buying_costs = [self.price_data.get_cost(op) for op in sell_ops] buy_op.buying_cost = misc.dsum(buying_costs) + assert len(sell_ops) == len(buying_costs) for sell_op, buying_cost in zip(sell_ops, buying_costs): assert isinstance(sell_op, tr.Sell) @@ -1668,9 +1719,7 @@ def resolve_trades_and_fees(self) -> None: log.warning(f"Matching trades failed ops={t_op}") # Only keep none fee operations in book. - self.operations = [ - op for op in self.operations if op.type_name != tr.Fee.type_name_c() - ] + self.operations = [op for op in self.operations if not isinstance(op, tr.Fee)] def read_file(self, file_path: Path) -> None: """Import transactions form an account statement. From b859c4a1205ae3a87c5ffe1b3ebbcd611e2cb7fb Mon Sep 17 00:00:00 2001 From: Jeppy Date: Sat, 4 Jun 2022 09:42:42 +0200 Subject: [PATCH 10/14] ADD Buy/Sell.unlinked property --- src/book.py | 22 +++++++++------------- src/transaction.py | 8 ++++++++ 2 files changed, 17 insertions(+), 13 deletions(-) diff --git a/src/book.py b/src/book.py index 25a298d..0b14196 100644 --- a/src/book.py +++ b/src/book.py @@ -1557,11 +1557,9 @@ def resolve_trades_and_fees(self) -> None: ] # type:ignore[assignment] assert all(isinstance(fee, tr.Fee) for fee in fees) - assert buy_op.link is None - assert buy_op.buying_cost is None + assert buy_op.unlinked buy_op.link = sell_op - assert sell_op.link is None - assert sell_op.selling_value is None + assert sell_op.unlinked sell_op.link = buy_op assert buy_op.fees is None buy_op.fees = fees @@ -1592,12 +1590,12 @@ def resolve_trades_and_fees(self) -> None: # Iterate over all ops and add links accordingly for buy_op in t_op[tr.Buy.type_name_c()]: assert isinstance(buy_op, tr.Buy) - assert buy_op.link is None + assert buy_op.unlinked for sell_op in t_op[tr.Sell.type_name_c()]: assert isinstance(sell_op, tr.Sell) - if sell_op.link is not None: + if not sell_op.unlinked: # Already matched. continue @@ -1605,7 +1603,7 @@ def resolve_trades_and_fees(self) -> None: bridge_coin ) == 1 if is_valid_pair: - assert sell_op.link is not None + assert sell_op.unlinked buy_op.link = sell_op sell_op.link = buy_op # Match fees to buy_op coin @@ -1624,8 +1622,8 @@ def resolve_trades_and_fees(self) -> None: else: raise RuntimeError("Unexpected behavior") - assert all( - op.link is not None + assert any( + op.unlinked for op in t_op.values() if isinstance(op, (tr.Buy, tr.Sell)) ), "Not all operations were matched." @@ -1698,16 +1696,14 @@ def resolve_trades_and_fees(self) -> None: assert len(sell_ops) > 0 assert all(isinstance(op, tr.Sell) for op in sell_ops) - assert buy_op.link is None - assert buy_op.buying_cost is None + assert buy_op.unlinked buying_costs = [self.price_data.get_cost(op) for op in sell_ops] buy_op.buying_cost = misc.dsum(buying_costs) assert len(sell_ops) == len(buying_costs) for sell_op, buying_cost in zip(sell_ops, buying_costs): assert isinstance(sell_op, tr.Sell) - assert sell_op.link is None - assert sell_op.selling_value is None + assert sell_op.unlinked percent = buying_cost / buy_op.buying_cost sell_op.selling_value = self.price_data.get_partial_cost( buy_op, percent diff --git a/src/transaction.py b/src/transaction.py index 8845a64..84d083c 100644 --- a/src/transaction.py +++ b/src/transaction.py @@ -173,11 +173,19 @@ class Buy(Transaction): link: Optional[Sell] = None buying_cost: Optional[decimal.Decimal] = None + @property + def unlinked(self) -> bool: + return self.link is None and self.buying_cost is None + class Sell(Transaction): link: Optional[Buy] = None selling_value: Optional[decimal.Decimal] = None + @property + def unlinked(self) -> bool: + return self.link is None and self.selling_value is None + class CoinLendInterest(Transaction): pass From 705d97f2d7e3a25c36cc67df4629a2990f108b84 Mon Sep 17 00:00:00 2001 From: Jeppy Date: Sat, 4 Jun 2022 09:52:16 +0200 Subject: [PATCH 11/14] ADD python module more_itertools --- dependencies/requirements.in | 3 ++- requirements.txt | 2 ++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/dependencies/requirements.in b/dependencies/requirements.in index a418568..fdbbfbe 100644 --- a/dependencies/requirements.in +++ b/dependencies/requirements.in @@ -1,4 +1,5 @@ python-dateutil==2.8.1 requests==2.25.1 xlsxwriter==3.0.3 -tzdata==2022.1 \ No newline at end of file +tzdata==2022.1 +more_itertools==8.13.0 \ No newline at end of file diff --git a/requirements.txt b/requirements.txt index 9d4ecb2..52a31cf 100644 --- a/requirements.txt +++ b/requirements.txt @@ -10,6 +10,8 @@ chardet==4.0.0 # via requests idna==2.10 # via requests +more-itertools==8.13.0 + # via -r dependencies/requirements.in python-dateutil==2.8.1 # via -r dependencies/requirements.in requests==2.25.1 From 66ee84cfcf5445edd9aad702a49d8e97eb8bccd9 Mon Sep 17 00:00:00 2001 From: Jeppy Date: Sat, 4 Jun 2022 09:53:04 +0200 Subject: [PATCH 12/14] UPDATE Check that utc_time stamps of bnb small asset transfers are roughly the same --- src/book.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/book.py b/src/book.py index 0b14196..681ad7e 100644 --- a/src/book.py +++ b/src/book.py @@ -23,6 +23,8 @@ from pathlib import Path from typing import Any, Optional +import more_itertools + import config import log_config import misc @@ -1696,6 +1698,13 @@ def resolve_trades_and_fees(self) -> None: assert len(sell_ops) > 0 assert all(isinstance(op, tr.Sell) for op in sell_ops) + # Check that all sell_ops occure at roughly the same time. + min_utc_time, max_utc_time = more_itertools.minmax( + sell_op.utc_time for sell_op in sell_ops + ) + diff = max_utc_time - min_utc_time + assert diff.total_seconds() < 1 + assert buy_op.unlinked buying_costs = [self.price_data.get_cost(op) for op in sell_ops] buy_op.buying_cost = misc.dsum(buying_costs) From 963d291a7067504f83a04273709c275391e1fa0d Mon Sep 17 00:00:00 2001 From: Jeppy Date: Sat, 4 Jun 2022 09:58:26 +0200 Subject: [PATCH 13/14] UPDATE format --- src/book.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/book.py b/src/book.py index 681ad7e..c510126 100644 --- a/src/book.py +++ b/src/book.py @@ -1624,12 +1624,14 @@ def resolve_trades_and_fees(self) -> None: else: raise RuntimeError("Unexpected behavior") + # Check that all operations were matched or raise an error. assert any( op.unlinked for op in t_op.values() if isinstance(op, (tr.Buy, tr.Sell)) ), "Not all operations were matched." + # Check that all fees were matched or raise a warning. linked_fees = set( op.fees for op in t_op[tr.Buy.type_name_c()] @@ -1645,6 +1647,7 @@ def resolve_trades_and_fees(self) -> None: f"Following fees were not considered: {', '.join(f.coin for f in fees)}. " "Please open an issue or PR." ) + continue # Binance allows to convert small assets in one go to BNB. @@ -1691,9 +1694,9 @@ def resolve_trades_and_fees(self) -> None: ] # Add previously cached sells to the list. - # TODO is it possible, that they occure after this timestamp - # check timestamps! sell_ops += bnb_small_asset_sell_cache + # Clear cache after adding it. + bnb_small_asset_sell_cache.clear() assert len(sell_ops) > 0 assert all(isinstance(op, tr.Sell) for op in sell_ops) @@ -1705,6 +1708,8 @@ def resolve_trades_and_fees(self) -> None: diff = max_utc_time - min_utc_time assert diff.total_seconds() < 1 + # Calculate buying cost and selling value of the + # small asset transfers as if they were one big trade. assert buy_op.unlinked buying_costs = [self.price_data.get_cost(op) for op in sell_ops] buy_op.buying_cost = misc.dsum(buying_costs) @@ -1717,8 +1722,7 @@ def resolve_trades_and_fees(self) -> None: sell_op.selling_value = self.price_data.get_partial_cost( buy_op, percent ) - # Clear cache after all sell ops are accounted for. - bnb_small_asset_sell_cache.clear() + continue log.warning(f"Matching trades failed ops={t_op}") From d1327dc553985ab7389155270fc7e3874decc2d3 Mon Sep 17 00:00:00 2001 From: Jeppy Date: Sat, 4 Jun 2022 10:01:03 +0200 Subject: [PATCH 14/14] FIX linting errors --- src/book.py | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/src/book.py b/src/book.py index c510126..4eccc6b 100644 --- a/src/book.py +++ b/src/book.py @@ -1529,7 +1529,8 @@ def resolve_trades_and_fees(self) -> None: # Cache for sell ops if the belonging buy op happens at a later utc_time. bnb_small_asset_sell_cache: list[tr.Sell] = [] - # Match trades which belong together (traded at same time) and add belonging fees. + # Match trades which belong together (traded at same time) and add + # belonging fees. for platform, _operations in misc.group_by(filtered_ops, "platform").items(): for _, matching_operations in misc.group_by( _operations, "utc_time" @@ -1610,8 +1611,10 @@ def resolve_trades_and_fees(self) -> None: sell_op.link = buy_op # Match fees to buy_op coin # BUG Fees paid in BNB are currently ignored. - # It is unclear which buy op the BNB-fee op should be accounted to. - # It is also possible to pay fees partly in the coin bought and partly in BNB + # It is unclear which buy op the BNB-fee + # op should be accounted to. + # It is also possible to pay fees partly + # in the coin bought and partly in BNB. fees = [ fee # type:ignore[misc] for fee in t_op[tr.Fee.type_name_c()] @@ -1644,7 +1647,8 @@ def resolve_trades_and_fees(self) -> None: ] if any(missing_fees): log.warning( - f"Following fees were not considered: {', '.join(f.coin for f in fees)}. " + "Following fees were not considered: " + f"{', '.join(f.coin for f in fees)}. " "Please open an issue or PR." ) @@ -1668,7 +1672,8 @@ def resolve_trades_and_fees(self) -> None: if is_binance_bnb_small_asset_transfer: if len(t_op[tr.Buy.type_name_c()]) == 0: # There are no buys. - # These sells might belong to a small asset transfer from another utc_time. + # These sells might belong to a small asset transfer + # from another utc_time. # Cache the sell operations. sell_ops: list[tr.Sell] = t_op[ tr.Sell.type_name_c() @@ -1686,9 +1691,10 @@ def resolve_trades_and_fees(self) -> None: else: (buy_op,) = t_op[tr.Buy.type_name_c()] assert isinstance(buy_op, tr.Buy) - assert ( - buy_op.coin == "BNB" - ), f"expected bnb small asset transfer, but coin is {buy_op.coin}" + assert buy_op.coin == "BNB", ( + "expected bnb small asset transfer, " + f"but coin is {buy_op.coin}" + ) sell_ops = t_op[ # type:ignore[assignment] tr.Sell.type_name_c() ]