Closed qwhelan closed 1 month ago
If you like you can run checks locally on your machine with make static_analysis check
.
@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:
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 https://github.com/eprbell/rp2/commit/195ed40a057550231c586f669505431bdf2438df.
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:
This should give us the best of both worlds. Do you have any concerns with this approach?
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.
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:
Under the current logic, an exception would be raised when processing the
SELL 0.2 BTC
transaction. This code, would raise an exception on theSELL 0.9 BTC
transaction instead. A subtler case looks like this: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.