> On Nov 13, 2021, at 8:41 PM, Lionel Élie Mamane <lio...@mamane.lu> wrote: > > Hi, > > I've started work on adding support to compute capital gains in > multi-currency situations; that is, when a lot from a > security-denominated account contains transactions with different > common currencies. > > I have a "it works" patch in bugzilla > https://bugs.gnucash.org/show_bug.cgi?id=798366 > Not very thoroughly tested in many scenarios, and see point 5 below. > > 1) It contains naked asserts, peeking at the rest of the code, I guess > I should replace them with g_assert > > 2) It changes the declaration (argument list / return type) of > xaccSplitGetCapGains > gnc_lot_get_balance_before > > These had only one caller in the GnuCash code each. > I'm not sure if those are considered public stable API? The Debian > packaging doesn't have separate libgnucash / libgnucash-dev > packages, and the .so files seem not to be soname-versioned? So > there is no concept of stable API at all in libgnucash? > > 3) Since the capital gain is not anymore in the same currency as every > split it is computed over, xaccSplitGetCapGains now needs to return > not only a value (a gnc_numeric), but also the associated currency. > > I created a struct for that, but I'm unhappy with the name. Any > better idea? And where to declare it? It sounds like it could > potentially be useful "throughout the code"? I'd call it "value_t" > or "amount_t" or some such (because a number without > unit/currency/commodity is meaningless, so in my mind a "value" or > an "amount" is with commodity), but this does not find with the how > these terms are used throughout the code... > > struct amount_s > { > gnc_numeric amount; > gnc_commodity *commodity; > }; > typedef struct amount_s amount_commodity; > > > 4) The design choice is that the capital gain is _always_ computed in > the currency of the first currency-denominated ancestor of the > security-denominated account, even when all the splits in the lot > are otherwise in a single other currency. This lets the user > control the behaviour through the account hierarchy: > > All investments under a EUR-denominated "investments" account: all > capital gains are in EUR, to follow the tax rule. > > Want to track different portfolios under different currencies > (e.g. because they belong to different branches in different > countries, or because that's more important than computing the > right capital gains according to tax rules)? Create a placeholder > account for each portfolio, denominated in the chosen currency, and > put the securities accounts as subaccounts of this one. > > > This will _change_ _behaviour_ from previous versions in the > aforementioned case (all the splits in the lot are otherwise in a > single other currency). I think making an exception, and computing > capital gains in the common currency of the splits in the lot (or > the whole account) when there is one, and in another currency when > there is no common currency in the lot, would be too messy > semantics, and a too "unstable" behaviour. By "unstable" I mean too > big behaviour changes in response to small changes in data. It > would also fail to serve the above scenarios which I have in mind. > > OTOH, the change of behaviour between versions is also not > attractive. > > What to do? Does GnuCash have a mechanism to mark files as > backwards-incompatible (e.g. a "minimum version needed" tag)? We > could then mark it so as soon as a capital gain is computed in a > currency different than any of the splits in the lot. > > Or, we could make the behaviour a per-file option, with: > > - The default for _new_ files being the new better behaviour. > - Any file where the option is not set (that is, an older file), is > considered as having the option set for the old behaviour. > - Any file that doesn't have the option set, the setting is _not_ > written on save (so as not to confuse older versions with an XML > tag they don't know). > > This keeps backwards and upwards compatibility; it does have the > disadvantage that all existing users must explicitly opt in to the > better behaviour. Maybe we could automatically opt in any file > where all already computed capital gains are in the currency that > the new behaviour would choose, too? > > > 5) Quite possibly my patch doesn't 100% deal correctly with scenarios > where the currency of the ancestor account changes; the code that > decides to mark capital gains as "dirty" needs to be made > currency-aware, and the code that changes the capital gains split > should reset the income account of the capital gains > transaction. Haven't looked into that yet. TOOD.
Sorry I didn't intervene sooner. You need to read through https://bugs.gnucash.org/show_bug.cgi?id=797796 <https://bugs.gnucash.org/show_bug.cgi?id=797796> before doing any more work because your approach doesn't support the GAAP/IAS requirements. Much of the rest of GnuCash doesn't either, but there's no point in doing a lot of work that doesn't move us in the direction of being legal. To your specific questions: 1. assert vs. g_assert: Doesn't really matter, they do the same thing. Neither is particularly useful in production code because they're macros that are disabled in release builds. They're useful for catching programming errors with unit tests. Your patch doesn't have any of those so there's no point in asserting anything. If you need to do things like null-checks to prevent crashes use g_return_if_fail or g_return_value_if_fail, which log a critical error and return, preventing the crash. 2. We make no stability guarantees for libgnucash at present. We have a long-term goal to do so, but the current API is rather haphazard and needs substantial clean up before we'd be willing to do so. It also has serious memory-management issues when used outside of the application so part of the required work is to rewrite the whole thing in C++, replacing the already incorrect GObject implementation. This has been discussed at great length over the last 10-12 years. If you're interested you can look through the mailing list archives. 3. There's already a struct combining commodity with gnc_numeric amount called gnc_monetary, declared in libgnucash/engine/gnc-commodity.h. 4a. The GAAP/IAS requirement is that all transactions get valued in the book's home currency on the day of the transaction using either an actual exchange rate if funds were converted that day or an average rate for the day if they weren't. One of the participants in bug 797796, CDB-Man, is a licensed accountant. The discussion on that bug is quite comprehensive and I won't attempt to summarize it. It's actually a pretty substantial simplification over what you've planned. 4b. Yes, GnuCash has a backward compatibility policy and a GncFeature component (libgnucash/engine/gnc-feature.[hc]) to test whether an older version of GnuCash can safely work with a book. 4c. Apropos the book property proposal, do you know about and understand Trading Accounts? Enabling use of trading accounts changes the behavior of the register in multi-commodity transactions. Neither cap-gains.c nor dialog-lot-viewer.c check for using trading accounts; perhaps they should. 5. I'm not sure what you mean here. If the code doesn't mark the splits dirty then they won't get saved in the SQL backends and all of your changes are for naught. If you mean that nothing marks the capital gain needing to be recomputed if the user edits the sell transaction that's OK, re-running the lot scrub from the dialog will find the imbalance and create an additional split. Some general observations: First, it will be far more productive for both of us if you'll resubmit your patch as a Github pull request. Code reviewing patch files in bug reports and keeping track of the changes is quite painful, and the larger the patch is the more painful managing the reviews and review comment corrections gets. No patch containing commented code is acceptable. Remove it instead. It makes the diff more understandable and prevents the accumulation of cruft. As evidenced by bug 797796 and the bugs you've created and commented on in the last few days this problem is quite a bit bigger than just the lot scrubber. Addressing it piecemeal, particularly with piecemeal design goals, will just add to the huge amount of technical debt we have already. Regards, John Ralls _______________________________________________ gnucash-devel mailing list gnucash-devel@gnucash.org https://lists.gnucash.org/mailman/listinfo/gnucash-devel
