dmitry-merzlyakov / nledger

.Net Ledger: Double-Entry Accounting System
Other
182 stars 51 forks source link

Error: Transaction does not balance #29

Closed alensiljak closed 1 year ago

alensiljak commented 1 year ago

Hi! Just tried NLedger on my book and am getting a Error: Transaction does not balance error. Using the same command with ledger, it parses just fine. I've been using Ledger for years and this is my main book.

What would be the most practical way to debug the issue? I guess I should narrow down to the smallest journal file that demonstrates it, somehow?

Thanks for creating this project. I'm quite interested in expanding on it.

alensiljak commented 1 year ago

Seems that there's an issue with calculating the capital gain/loss. The case is the following:

2017-01-13 Sell ABC
    ; sold at a loss
    Assets:Investments:ABC  -1945.83 ABC {1.3479 XYZ} [2008-04-17] @@ 2500 XYZ
    Income:Investment:Capital Gains      122.78 XYZ
    Assets:Investments:Cash         2500.00 XYZ

The finalization of a Xact seems to simply take the Cost (-2500) and then compare to the resulting Cash (2500) + the Capital Gains (122.78). Of course, this does not match. However, it also knows that 1945.83 ABC units times the price of 1.3479 XYZ equal 2,622.78 XYZ. Hence there is a loss of (-)122.78 XYZ in the first Posting. This is the difference to the 2500 XYZ received on sale. However, Ledger calculates the sale of -1945.83 units of ABC for the total of 2500 XYZ.

alensiljak commented 1 year ago

The full error is

Unbalanced remainder is:
           -0.00 XYZ
Amount to balance against:
        2,622.78 XYZ
Error: Transaction does not balance
alensiljak commented 1 year ago

Actually, the calculation of the Capital Gain/Loss is fine. The problem is in https://github.com/dmitry-merzlyakov/nledger/blob/624cb671e9fc860057611df84cba1484331c4ec5/Source/NLedger/Xacts/XactBase.cs#L368.

The balance at the end of finalization is -0.00 XYZ. However, the IsZero does not resolve to True. It is false!

?balance.IsZero
false

That is a bit weird, even for dotnet!

alensiljak commented 1 year ago

So, the actual problem seems to be https://github.com/dmitry-merzlyakov/nledger/blob/624cb671e9fc860057611df84cba1484331c4ec5/Source/NLedger/Amounts/Amount.cs#L956C25

IsRealZero expects the Amount to have a positive Sign, for whatever reason, even though the Amount is 0.

Or, rather,

https://github.com/dmitry-merzlyakov/nledger/blob/624cb671e9fc860057611df84cba1484331c4ec5/Source/NLedger/Amounts/Amount.cs#L969

Quantity.HasValue is still True.

alensiljak commented 1 year ago

So, the problem is with the BigInt implementation. I wonder how this has not come up in the tests.

alensiljak commented 1 year ago

Yes, unfortunately, the logic here is not correct...

https://github.com/dmitry-merzlyakov/nledger/blob/624cb671e9fc860057611df84cba1484331c4ec5/Source/NLedger/BigInt.cs#L100

Either HasValue should be reset to False when the amount is 0, or the check in the Amount should return True.

alensiljak commented 1 year ago

Here's the sample Ledger file:

; This errors out in NLedger
; NLedger-cli -f d:\tmp\error.ledger b

2008-07-17 Buy ABC
    ; 2,622.78 XYZ
    Assets:Investments:ABC  1945.83 ABC @ 1.3479 XYZ
    Assets:Investments:Cash

2017-01-13 Sell ABC
    ; sold at a loss
    Assets:Investments:ABC  -1945.83 ABC {1.3479 XYZ} [2008-07-17] @@ 2500 XYZ
    Income:Investment:Capital Gains   122.78 XYZ
    Assets:Investments:Cash      2500.00 XYZ
dmitry-merzlyakov commented 1 year ago

Hi, I am so sorry for delayed response. GitHub did not send notification about this issue for some reason, so I completely missed all these messages. Let me take time other the weekend to check all this stuff, thanks.

alensiljak commented 1 year ago

Thanks for the feedback! I was actually looking forward to trying to solve this one. As mentioned in the other issue, I'd be interested to learn more and get involved in the project, to a reasonable degree. If you could share any insights about the reasoning for the BigInt implementation and the HasValue, that would be helpful. Additionally, it might be worth to consider if there are types that could replace this now. There are nullable types, and int64, that might cover the use cases of the custom BigInt implementation. But maybe I'm missing something. I have not had a big look at the codebase yet. Still in the early stages of looking what is there to build on instead of reinventing the wheel.

dmitry-merzlyakov commented 1 year ago

FYI - the issue is confirmed; I see the difference in behavior between NLedger and Ledger (exactly as described). I need to take time to investigate it deeper; will keep you posted, thanks.

dmitry-merzlyakov commented 1 year ago

@alensiljak Hi, FYI - the issue is fixed; the fix is added to next-dev-0.8.5-net6 branch. Please, let me know if you want me to publish the fix asap - in this case, I will cherry-pick the fix to next-dev-0.8.5 and issue this version as it is (without waiting for completion of other tasks). Otherwise, it will be published when multi-targeting work is done.

The fix adds a test file based on your example. I also noticed that one of internal unit tests reflected incorrect behavior; it was fixed as well.

Thanks.

alensiljak commented 1 year ago

Great, thank you! Good to see it resolved quickly. Also looking forward to learning what the issue was. Do you have an idea when approximately would you have the new version ready under normal circumstances? I can build a new version for my testing. And the integration can also move with the currently published version, so it is not urgent. But, of course, having a new version published would be welcome in the near future.

dmitry-merzlyakov commented 1 year ago

@alensiljak Hi, you can check the commit description; it briefly explains the matter of the problem. Basically, it was a typo - I did not re-assign an immutable object after changes by a transformation function. Apparently, this case was not revealed by existing test till you provided your sample. I checked references - it seems like it was only such a mistake with that function. If you are about an approach to troubleshoot similar defects - I rebuilt a corresponded version of c++ ledger and compared behaviors under debugging.

About realistic terms to issue a new version - I reviewed the amount of pending planned work; it is purely about PowerShell scripts and related stuff. The changes do not seem to be critical, but the scope is noticeable. So, I would think about the middle of December or so - definitely before the end of year (I will limit scope if I find that things become uncontrollable). Therefore, if you want to have fixed library published earlier - I can issue it with fixes and current targets on the next week. Thank you!

alensiljak commented 1 year ago

Thanks for the pointer. I was also trying to understand the Ledger logic using a C++ debugger in vscode but could not follow many control flows or see the values of the variables. Hence I did not understand the details, especially the tagged values, which I could not even see in the debugger. I tried LLDB and g++, I think. Not really what I expected to see, though.

In any case, the timeline is quite OK. No need to stress. As mentioned, I may play around with the custom-built version in the meantime. I am still focusing on rewriting Cashier and completing the functionality required when I'm away from the computer. Currently that's mostly the balance display and transaction capture "on-the-go". Considering everything is going smoothly, I'm looking forward to incorporating NLedger into it. I need to work out some issues since OPFS is different to the regular file system and I need to find a solution for includes, for example. In any case, thank you for the prompt fix!

alensiljak commented 1 year ago

Wonderful, this indeed fixes the issue. I can confirm now being able to get the balance report!

There are slight differences in string ordering, though. Not that I'm nitpicking but I'm surprised to see Ledger using the Unix-style ordering, where a capital letter has a precedence to a lowercase letter. The account balances are all correct, however. Thanks a lot!