Add more output to acquired lots exhausted exception.

[jayr0d]

The current output of AcquiredLotsExhaustedException is not helpful enough to the user. If they missed a transaction, had a typo in a transaction, or rounding errors are accumulating from the data provided by the exchange, the user will need to manually process their transaction history to find when the exception occurred.

The new output of AcquiredLotsExhaustedException will provide the calculation result and the offending location in the transaction history.

black src tests reformatting is also included.

[eprbell]

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.

[jayr0d]

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.

[eprbell]

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.

[jayr0d]

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.

[jayr0d]

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

[eprbell]

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).

[jayr0d]

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?

[eprbell]

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).

[jayr0d]

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

[jayr0d]

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!