Improve precision of which transaction caused balance to go negative

[qwhelan]

The existing logic accumulates all buys first before decrementing from sells. As a result, it can only detect negative balances that persist until the end of the calculation period. It also means that the blamed transaction can be incorrect. Consider the following transaction sequence:

BUY 0.5 BTC
SELL 0.9 BTC
BUY 0.5 BTC
SELL 0.2 BTC

Under the current logic, an exception would be raised when processing the SELL 0.2 BTC transaction. This code, would raise an exception on the SELL 0.9 BTC transaction instead. A subtler case looks like this:

BUY 0.5 BTC
SELL 0.9 BTC
BUY 0.5 BTC
SELL 0.1 BTC

The current code would not raise any exception, despite going significantly negative for a period. The new code would correctly flag the invariant being violated.

[eprbell]

If you like you can run checks locally on your machine with make static_analysis check.

[qwhelan]

@eprbell Thanks, static analysis should be passing.

Completely forgot to mention this when opening the PR - a lot of test data fails this check now, which leads to a bunch of failing tests.

Easiest way would be to turn the check off for these tests, but there's a chance other issues are being hidden by the test data being off. For example, tab B2 of input/test_data.ods now has this failure in tests/test_tax_engine.py::TestTaxEngine::test_good_input:

E                   rp2.rp2_error.RP2ValueError: B2 balance of account "Coinbase" (holder "Bob") went negative (-0.20000000000) on the following transaction: OutTransaction:
E                     id=15
E                     timestamp=2020-02-11 19:58:00.000000 +0000
E                     asset=B2
E                     exchange=Coinbase
E                     holder=Bob
E                     transaction_type=TransactionType.SELL
E                     spot_price=12200.0000
E                     crypto_out_no_fee=1.00000000
E                     crypto_fee=0.00000000
E                     unique_id=
E                     is_taxable=True
E                     fiat_taxable_amount=12200.0000

Relevant cells showing the error to be correct are highlighted in yellow:

[eprbell]

I still need to look at the code of this PR (I'll do that in the weekend or sooner), but this is a good check, so thanks for identifying and fixing this problem!

In fact I had already found that some tests end up with a negative balance (even without this fix) and added the -n switch to disable the check for negative balances: see 195ed40.

I imagine this PR probably causes even more tests balances to go negative, however I'm reluctant to change the tests: the reason is that I did a lot of manual verification of these tests and they estabilish a strong baseline against regression (the balance may go negative here and there, but the tax engine logic is still tested thoroughly by these tests).

So my suggestion is to:

• disable negative balance checks (only for the tests that run into the issue), similar to what happens here: https://github.com/eprbell/rp2/blob/main/tests/test_ods_output_diff.py#L37
• write new tests running with this check enabled and testing negative balances specifically (I think you already added some of these tests).

This should give us the best of both worlds. Do you have any concerns with this approach?

[qwhelan]

Makes sense as an approach - my main concern is having a giant PR that's infeasible to work with. Hopefully it's feasible to land smaller bits using your -n switch to keep things tractable. I'm going to be out of town this weekend, so might be a bit before I take a stab at things (and might submit some other unrelated PRs first).

Just for an order-of-magnitude, I'm seeing 47 tests passing, 1 failed, and 66 errors. The latter bucket seems to all be ODS output diff style tests.

[eprbell]

I did a first review: looks good. After we fix the tests, as discussed above, this can be merged.

[eprbell]

Could this be List[AbstractTransaction] instead of a Union?

[qwhelan]

Solved this in the most recent commit via this approach: https://peps.python.org/pep-0484/#type-variables-with-an-upper-bound.

This can be simplified when 3.11 is the minimum supported version via PEP 673

[eprbell]

This looks great: I have just a couple of nits and then we can merge this.

[eprbell]

LGTM. Thanks for fixing this!