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.

Sign up for GitHub

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

Jump to bottom

Add more output to acquired lots exhausted exception. #110

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add more output to acquired lots exhausted exception. #110

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
536f30f
black src tests reformatted
jayr0d Apr 4, 2024
7e819ae
Add more output to AcquiredLotsExhaustedException.
jayr0d Apr 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions src/rp2/balance.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@
from rp2.rp2_decimal import ZERO, RP2Decimal
from rp2.rp2_error import RP2TypeError, RP2ValueError


CRYPTO_BALANCE_DECIMAL_MASK: Decimal = Decimal("1." + "0" * 10)


Expand Down Expand Up @@ -146,7 +145,7 @@ def __init__(
):
raise RP2ValueError(
f'{intra_transaction.asset} balance of account "{from_account.exchange}" (holder "{from_account.holder}") went negative '
f'({final_balances[from_account]}) on the following transaction: {intra_transaction}'
f"({final_balances[from_account]}) on the following transaction: {intra_transaction}"
)

# Balances for sold and gifted currency
Expand All @@ -164,7 +163,7 @@ def __init__(
):
raise RP2ValueError(
f'{out_transaction.asset} balance of account "{from_account.exchange}" (holder "{from_account.holder}") went negative '
f'({final_balances[from_account]}) on the following transaction: {out_transaction}'
f"({final_balances[from_account]}) on the following transaction: {out_transaction}"
)

for account, final_balance in final_balances.items():
Expand Down
8 changes: 5 additions & 3 deletions src/rp2/gain_loss_set.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,9 +204,11 @@ def _sort_entries(self) -> None: # pylint: disable=too-many-branches
LOGGER.debug(
"%s (%d - %d): taxable event housekeeping",
last_gain_loss_with_acquired_lot.internal_id,
current_acquired_lot_fraction[last_gain_loss_with_acquired_lot.acquired_lot]
if last_gain_loss_with_acquired_lot.acquired_lot in current_acquired_lot_fraction
else 0,
(
current_acquired_lot_fraction[last_gain_loss_with_acquired_lot.acquired_lot]
if last_gain_loss_with_acquired_lot.acquired_lot in current_acquired_lot_fraction
else 0
),
current_taxable_event_fraction,
)

Expand Down
5 changes: 4 additions & 1 deletion src/rp2/tax_engine.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,10 @@ def _create_unfiltered_gain_and_loss_set(
)

except AcquiredLotsExhaustedException:
raise RP2ValueError("Total in-transaction crypto value < total taxable crypto value") from None
raise RP2ValueError(
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this is not quite correct: line 196 doesn't capture acquired_lot_amount - taxable_event_amount < 0 (this is already captured above at line 179 and is a condition related to an individual couple of acquired lot and taxable event). Line 196 captures the fact that acquired lots have been exhausted for the given taxable event using the given accounting method, so it's not even possible to try with the next acquired lot, because there are no more (unlike what happens at line 176).

When running into this, the solution is to enable debug logs (by prepending the command with LOG_LEVEL=DEBUG) and carefully analyze the various logs starting with "tax_engine: " (see above in this file). This will lead to pinpointing the problem.

I think this is an important issue though: it probably deserves a user FAQ.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback. I will look into this further (run through debug logs for a specific case) when I have a chance.

It might be good to give some user direction in the exception output, even if that is a redirection to LOG_LEVEL=DEBUG. In my case I did find the time and value at the time of occurrence useful, but it sounds like I at least got the value wrong.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think this PR could change to:

  • new user FAQ with the description of the problem and the procedure to solve it;
  • add a pointer to the new FAQ to the error message.

Copy link
Contributor Author

@jayr0d jayr0d Apr 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please consider the following and let me know your thoughts.

Setting an environment variable varies by environment. In PowerShell for example, it needs to be set via a different execution line, such as:
$Env:LOG_LEVEL = "DEBUG"

I modified my data set to reintroduce one of the issues I cited and ran with LOG_LEVEL=DEBUG. If we are going to redirect the user to debug logging, they will need to navigate to the end of the log file and look for the DEBUG: tax_engine as you suggest, where the last entry will be taxable > acquired. It is likely there will be many other entries with id=y->x as it searches for acquired lots to meet the taxable out transaction. In my case, there were almost 300 lots (x) to try and account for y. There is also no direct output to the user the amount they are negative by; the user needs to calculate it manually from the last "DEBUG: tax_engine: taxable > acquired ..." output.

The change I made does output the transaction information for the correct id (y). I think it does well to point the user in the right direction. Each time I had this problem I needed to know when/where I went negative and scrutinize in that area, and this clearly points that out.

I recommend still printing the offending output transaction information when this condition is met, since it is a fatal exception and I think most of the time this will be enough information for the user to determine what the problem is and they will not actually need to turn on debug. I think this is a more user friendly approach and that is my goal, to make transitioning to DaLI and RP2 easier.

Whether you want the output transaction in the normal output or not, I can take a stab at modifying the output to refer to an FAQ section where both DEBUG and review of the DEBUG logs can be described.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this a bit more it does seem the values I am populating for "Total in-transaction crypto value - total taxable crypto value" are for the individual taxable and acquired lot portion, and not the total. Maybe something like:

Taxable out-transaction crypto value > acquired in-transaction crypto value by 0.012345 for OutTransaction:
  id=y
  ...

For more information, see Some Section in the User FAQ: https://github.com/eprbell/rp2/blob/main/docs/user_faq.md#somenewsection

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way I see it is that the CSV files are meant as an "in progress" version of the "Gain / Loss Detail" table in the rp2_full_report.ods output file. So they should contain similar data. We could have one CSV file per asset to keep things easy for the user.

I suggest appending one new CSV row for the current gain loss entry at this location: https://github.com/eprbell/rp2/blob/main/src/rp2/gain_loss_set.py#L129 (just before the "if" statement).

For reference you can look at _fill_cell calls here: https://github.com/eprbell/rp2/blob/main/src/rp2/plugin/report/rp2_full_report.py#L802 (it's where the columns of the "Gain / Loss Detail" table are filled).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the pointers, I certainly don't know the best place in the code to insert new behavior.

I was not suggesting one CSV per asset. One asset should not have direct impact on the other. Also, since this error condition results in termination, not all assets will be processed.

If the intent is to show the user as much information as possible (all assets, not just the one that ran into the error), and the intent is to show the user information in a manner similar to the Asset Tax sheet in the full report, I wonder if it is just best to ignore the difference and keep going, and reporting it to the user (something like an auto-balance I was suggesting). They will know there is an issue to look into, they will get the output in a similar manner as they would the full report, and they will get all of the other features like being able to click around and navigate throughout the ODS report. No need to use CSV files with no "features".

I think in either case it is beneficial to add a balance column to the In-Out sheet and now I guess the Tax sheet as well, since Crypto (In/Out/Intra/Amt) Running Sum is in both of those.

Thoughts?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting thought... this will need to be experimented with a bit. We need to understand if there are any side effects of not interrupting execution. I guess to start with we can change the exceptions that are raised in tax_engine.py:_create_unfiltered_gain_and_loss_set() to a new custom exception (e.g. RP2AccountingMethodException) and capture that in the caller, which is compute_tax() in the same file.

Let's do the balance column addition in a separate PR to keep things simple (that will require updating the golden files).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I will work on the balance column addition first, probably not this week.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a working prototype for in, out, and intra balance columns. Gain/loss is not being calculated correctly. I think once I get that working I might ask for an initial review before I start updating automated testing, gold, etc. More work than I expected and it is now the nice weather months so this might take a bit!

f"Total in-transaction crypto value ({acquired_lot_amount}) - total taxable crypto value ({taxable_event_amount}) went negative "
f"({acquired_lot_amount - taxable_event_amount}) on the following event: {taxable_event}"
) from None
except TaxableEventsExhaustedException:
pass

Expand Down
19 changes: 18 additions & 1 deletion tests/test_tax_engine.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,12 +123,29 @@ def test_bad_input(self) -> None:
)
)

with self.assertRaisesRegex(RP2ValueError, "Total in-transaction crypto value < total taxable crypto value"):
with self.assertRaises(RP2ValueError) as value_error:
compute_tax(
self._good_input_configuration,
self._accounting_engine,
input_data,
)
self.assertEqual(
str(value_error.exception),
"""Total in-transaction crypto value (4.98000000000) - total taxable crypto value (21.2) went negative (-16.22000000000) on the following event: \
OutTransaction:
id=38
timestamp=2020-06-01 03:59:59.000000 -0400
asset=B4
exchange=Coinbase Pro
holder=Bob
transaction_type=TransactionType.SELL
spot_price=900.9000
crypto_out_no_fee=20.20000000
crypto_fee=1.00000000
unique_id=
is_taxable=True
fiat_taxable_amount=18198.1800""",
)


if __name__ == "__main__":
Expand Down
Loading