GNOME Bugzilla – Bug 537476
Implement currency trading accounts
Last modified: 2018-06-29 22:05:24 UTC
It would be nice if Gnucash implemented currency trading accounts as described in Peter Selinger's document at <http://www.mathstat.dal.ca/~selinger/accounting/gnucash.html>. I have partially implemented these changes (enough to be useful). Since it appears unlikely that these will be integrated into Gnucash, and since I have gotten a number of requests for these patches, I am creating this Bugzilla entry to make them available. The parts of Gnucash that are patched for this have remained fairly stable recently, but I will attempt to keep the patches up to date as Gnucash changes.
Created attachment 112438 [details] [review] Patch to implement commodity trading accounts This patch implements trading accounts somewhat as described in Peter Selinger's document at <http://www.mathstat.dal.ca/~selinger/accounting/gnucash.html>. Although he describes it as a multiple currency problem, it really applies to any transactions involving multiple commodities (for example buying or selling a stock) Hence I've called the trading accounts "commodity exchange accounts" which seems more descriptive. In summary these patches add an option to use commodity exchange accounts and if it is on a transaction must be balanced both in value (in the transaction currency) and in each commodity or currency used in any split in the transaction. If a transaction only contains splits in the transaction currency then this is the same rule as Gnucash has always enforced. If necessary Gnucash will create splits in commodity exchange accounts to balance the transaction in each commodity. It will create the commodity exchange accounts in the hierarchy "Trading:namespace:commodity" where "Trading" is a new top level place-holder account (whose name is localizable). These accounts are created as "Income" accounts. It would be better to create a new account type for them, but so far I haven't done that. See 16 below. If the option to use commodity exchange accounts is on and a transaction contains splits in multiple commodities, the register will show the amount in the split's commodity instead of the value in the transaction's currency for splits not in the transaction currency. (See change 9 below.) This makes it possible to edit the amounts for these splits. The current register code still doesn't work completely correctly with these changes. The register rewrite currently taking place should improve things in this regard. It is still necessary to set exchange rates correctly when entering multiple-commodity transactions (rather than setting them all to 1 as described in Peter Selinger's document. This is because it is necessary for the transaction to balance in value as well as in each commodity. Changing things so transactions balance only in commodity and not in value would break too many things in Gnucash, and it seems like a bad idea anyway. Some possible improvements to this include *** Make all this work with the register rewrite. *** Make the "Use trading accounts" option an attribute of the data file, not a global preference. *** Make the parent trading account (currently a top level account called "Trading") configurable. *** Make the commodity trading accounts a new account type instead of being Income accounts. A more detailed list of changes follows: 1. Added a "Use commodity exchange accounts" option. 2. Added gnc_monetary and MonetaryList data types. 3. Renamed xaccTransGetImbalance to xaccTransGetImbalanceValue and added a new xaccTransGetImbalance that returns a MonetaryList. Also added xaccTransIsBalanced to see if the transaction is balanced without returning a GList that needs to be freed. It calls both xaccTransGetImbalance and xaccTransGetImbalanceValue since a transaction may be unbalanced with regard to either without being unbalanced with regard to the other. 4. Changed gnc_split_register_get_debcred_bg_color to use xaccTransIsBalanced. 5. Changed gnc_split_register_balance_trans to not offer to adjust an existing split if there imbalances in multiple currencies. Because of bugs in the register code this is rarely called. 6. Changed importers to use xaccTransGetImbalanceValue to check for imbalance, assuming that they won't create multiple currency trasactions. 7. Changed xaccTransScrubImbalance to create a balancing split for each imbalanced commodity in the transaction. Also balances the transaction value. The commodity balancing splits go into accounts in the hierarchy Trading:NAMESPACE:COMMODITY. The value balancing splits go into Imbalance-CURRENCY as before. 8. Changed xaccSplitConvertAmount to use xaccTransIsBalanced instead of xaccTransGetImbalance. 9. Changed gnc_split_register_get_debcred_entry to sometimes use the split amount instead of value if using currency accounts. If the register is a stock register (i.e., shows shares and prices), it uses the value if the split is in the register commodity (i.e. is for the stock) and the amount otherwise. It shows the currency symbol unless the commodity is the default currency. If the register is not a stock register it always uses the amount and shows the currency symbol if the split is not in the register commodity. Also changed it to not return a value for a null split unless the transaction is unbalanced in exactly one currency. This is what goes in a blank split as the proposed value. 10. Changed refresh_model_row to use xaccTransGetImbalanceValue to get the imbalance, assuming that importers don't create transactions in multiple currencies. Also same change in gnc_import_process_trans_item, downloaded_transaction_append, and gnc_import_TransInfo_is_balanced. 11. Changed the TRANS_IMBALANCE accessor method in xaccTransRegister to use xaccTransGetImbalanceValue. As far as I can tell this is only used by the "pd-balance" query type in gnc_scm2query_term_query_v1() defined in engine-helpers.c. This query type only tests the result for zero/non-zero. 12. Changed xaccTransGetAccountConvRate to accept any split into the correct commodity instead of insisting on one into the provided account. Then can use it in xaccTransScrubImbalance to set the value of the imbalance split from its amount, however later changed xaccTransScrubImbalance to not use it. Instead it sets the value for the new split correctly to keep the value of the whole transaction balanced. 13. Changed the balance sheet report to include a new option to not compute unrealized gains and losses. 14. Related to 9 above, changed gnc_split_register_auto_calc to not do anything if given a stock register where the value cell doesn't contain the value. 15. Also related to 9, changed gnc_split_register_save_amount_values to set the amount and value fields in the split correctly when using trading accounts. 16. Changed the new account and edit account dialogs to allow any commodity or currency for an income account if using trading accounts. It would be better to add a new account type for trading accounts, but that's a big deal and I'll leave that for later after we see whether this set of changes is going to be accepted or rejected. 17. Change gnc_xfer_dialog_run_exchange_dialog to understand that the new value is really the split's amount if using trading accounts. 18. Changed xaccSplitGetOtherSplit to ignore trading splits if using commodity exchange accounts.
For the record, I think Peter's ideas are quite convincing and this kind of "commodity trading accounts" should indeed be implemented. However, this didn't meet enough consensus on gnucash-devel, so for now it won't go into SVN trunk. (But promoting the patch to "reviewed" because it looks good to me.)
Created attachment 143629 [details] [review] Updated patch for commodity trading accounts This version of the patch is for Gnucash version 2.3.5 (SVN r18284). It differs from the previous patch in one respect (in addition than being for a later version of Gnucash). In this version commodity trading accounts are a new account type called a "Trading" account. In the previous version of the patch they were Income accounts which wasn't really correct. This change makes it easier for them to be treated properly by various reports and other parts of Gnucash. In particular the balance sheet report has been changed to handle trading accounts properly, or at least better than it did before. Please let me know if it isn't handling them correctly. Changing the account type affected several files and as a result this patch is somewhat bigger than the previous one. Unfortunately, this change will probably make this patch even more unacceptable to mainstream Gnucash developers since adding a new account type will likely make the saved file or data base incompatible with previous versions of Gnucash. Perhaps it would be possible to get a change in the 2.2.x series to treat trading accounts like income accounts so files created by 2.3.x could be read by 2.2.x. This would be trivial if it weren't necessary to round-trip the data since one could just change the account type when the account was loaded. Making things round-trip correctly would add some complexity. On the other hand, since the use of trading accounts is entirely optional and by default Gnucash won't create them, we could just say that if you want to use that feature of 2.3.x, you won't be able to go back to 2.2.x. If you don't turn on that feature, your files remain compatible with 2.2.x. This would be my suggestion.
Comment on attachment 143629 [details] [review] Updated patch for commodity trading accounts Similar to comment #2, I still think this approach is convincing and a good solution to the actual problem. I agree it is fine to say "If you use this new feature, you can't load the data file with old gnucash.", which is what can be expected from a new version.
Comment on attachment 143629 [details] [review] Updated patch for commodity trading accounts (changing status to commit_now because IMHO it indeed can be committed. Status as "reviewed" makes this bug report drop from the easily accessible searches.)
I tried to apply it and got: phil@phil-laptop:~/gnucash2/trunk$ patch -p0 < currency.diff patching file src/register/ledger-core/split-register-model-save.c patching file src/register/ledger-core/gnc-ledger-display.c patching file src/register/ledger-core/split-register-model.c Hunk #1 succeeded at 152 (offset 61 lines). Hunk #2 succeeded at 674 (offset 63 lines). Hunk #3 succeeded at 1450 (offset 82 lines). Hunk #4 succeeded at 1516 (offset 82 lines). Hunk #5 FAILED at 1584. Hunk #6 FAILED at 1665. 2 out of 6 hunks FAILED -- saving rejects to file src/register/ledger-core/split-register-model.c.rej patching file src/register/ledger-core/split-register.c Hunk #3 FAILED at 2251. 1 out of 3 hunks FAILED -- saving rejects to file src/register/ledger-core/split-register.c.rej patching file src/register/ledger-core/split-register.h patching file src/register/ledger-core/split-register-layout.c Hunk #2 succeeded at 450 (offset 1 line). (the rest succeeded). Please update so that it applies cleanly.
It applies cleanly against version 2.3.5, but there are a lot of changes in SVN since then. I'm back in town for a few days between trips and will update the patch if I have time. Otherwise it won't get updated until the end of the month. Sorry about that, but I have a lot going on right now.
Created attachment 146545 [details] [review] Revised patch corresponding to r18399 This version of the patch differs from the previous one in one significant respect. The option to use trading accounts has been moved from Edit->Preferences to File->Properties and is now associated with the active book instead of being a global option. If you have set the global value on in a previous version you will need to set it on again in each file for which you want trading accounts, the previous global setting will be ignored.
The patch in attachment 146545 [details] [review] won't work right unless bug #599953 is fixed, either by applying the patch there or some other fix. Without that fix the new book local option can never be turned on.
Comment on attachment 146545 [details] [review] Revised patch corresponding to r18399 I guess this is fine for inclusion in trunk.
Comment on attachment 146545 [details] [review] Revised patch corresponding to r18399 r18429, thanks a lot!
@Mike: The current patch has a problem: At startup, gnucash here at my machine crashes with Backtrace: In unknown file: ?: 0* gnc:*book-label* <unnamed port>: In expression gnc:*book-label*: <unnamed port>: Unbound variable: gnc:*book-label* and as a workaround I've disabled the lookup in qofbook.c in http://svn.gnucash.org/trac/changeset/18430 , just to have a non-crashing trunk for the time being. Could you provide a fix for this problem? Thank you very much in advance :-)
This (and gnc:*trading-accounts*) is defined in business-utils.scm which is part of the optional business module. I didn't realize it was optional when I put it there and I have it installed so it worked for me. Unfortunately all of the book-based properties code is in the business module so I'll have to split out the code that manages the trading accounts option and put it somewhere else. Fixing this properly while still making the business module optional is not trivial. I'm in the process of switching machines right now (a process which is not going well) and am not in a position to make a change of this magnitude. I'll get to it as soon as possible.
I think this is causing 'make check' to fail, which will stand in the way of 2.3.8. Please either 1) fix or 2) revert until a fix is available.
Fail in what way? I think that things may be in a somewhat inconsistent state after r18430. On the other hand it's possible that make check needs to be updated to work with the trading accounts change. Today is a major holiday in the US and I haven't had time to work on this, but I should be able to get to it tomorrow or Saturday.
A number of the engine tests are failing. /bin/bash: line 5: 13571 Segmentation fault SRCDIR=. GNC_MODULE_PATH="../../../src/engine/.libs:${GNC_MODULE_PATH}" GUILE_LOAD_PATH="../../../src/gnc-module:../../../src/engine:${GUILE_LOAD_PATH}" LD_LIBRARY_PATH="../../../src/engine/.libs:../../../src/gnc-module/.libs:../../../src/core-utils/.libs:../../../lib/libqof/qof/.libs:${LD_LIBRARY_PATH}" DYLD_LIBRARY_PATH="../../../src/engine/.libs:../../../src/gnc-module/.libs:../../../src/core-utils/.libs:../../../lib/libqof/qof/.libs:${DYLD_LIBRARY_PATH}" ${dir}$tst FAIL: test-transaction-reversal phil@phil-laptop:~/gnucash2/trunk/src/engine/test$ gdb .libs/test-transaction-reversal GNU gdb (GDB) 7.0-ubuntu Copyright (C) 2009 Free Software Foundation, Inc. License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html> This is free software: you are free to change and redistribute it. There is NO WARRANTY, to the extent permitted by law. Type "show copying" and "show warranty" for details. This GDB was configured as "i486-linux-gnu". For bug reporting instructions, please see: <http://www.gnu.org/software/gdb/bugs/>... Reading symbols from /home/phil/gnucash2/trunk/src/engine/test/.libs/test-transaction-reversal...done. (gdb) run Starting program: /home/phil/gnucash2/trunk/src/engine/test/.libs/test-transaction-reversal [Thread debugging using libthread_db enabled] Program received signal SIGSEGV, Segmentation fault. 0x008feb1b in scm_igc () from /usr/lib/libguile.so.12 (gdb) bt
+ Trace 219322
For now, I've committed a change which removes the body of qof_book_use_trading_accounts() and has it just return FALSE. This will allow 'make check' to pass so that 2.3.8 can be released. After 2.3.8 has been released (which will be in a day or so), this change can be reversed.
Created attachment 148705 [details] [review] Patch to fix problems in comments 12 and 14 This patch should fix the problems described in comments 12 and 14. At least make check now works for me and the code that crashed in comment 12 is gone. There are a few values related to the trading accounts preference that are needed in both Scheme and C code. Since one of them was already defined in Scheme before I started, I defined all of them there and tried to import them to C. This is obviously not a good idea, especially since the make check tests don't even fire up Guile so Scheme code isn't available. This patch changes things around to define the values in C and import them to Scheme. Note that I added a new file, src/libqof/qof/qofbookslots.h, since I couldn't figure out where to put the C definitions and have them imported by Swig without either violating include hierarchies or having Swig import too much extraneous junk. If someone has a better idea, move the #defines if you want to.
I believe that changes related to this bug are causing my Balance Sheet report to not balance. I see "Trading Gains" listed in the equity section, and "Total Liabilities & Equity" is too high by twice the "Trading Gains" amount. Ideas?
If you've turned on the trading accounts option and scrubbed all transactions that use multiple commodities or currencies to actually use trading accounts, then you need to turn off the option "Compute unrealized gains and losses" in the general panel of the options for the Balance Sheet report. The report code tries to do what trading accounts do and you have to tell it not to if you've correctly used trading accounts in all transactions since they record unrealized gains. I probably should remove the option from the report and just use the book-level "use trading accounts" option to control this behavior.
Comment on attachment 148705 [details] [review] Patch to fix problems in comments 12 and 14 I think the three strings in qofbookslots.h need to be marked for translation by N_("...") so that they appear in gnucash.pot. Apart from that, the code is IMHO ready for inclusion already right now.
Trading accounts is off for me. I get an unbalanced balance sheet both with and without "Compute unrealized gains and losses."
Forest, > Trading accounts is off for me. I get an unbalanced balance sheet both with > and without "Compute unrealized gains and losses." Are you sure that you manually accounted for your cap gains/losses?
( also, are you sure you have a reasonable setting for price quote source? ) Honestly, this discussion should move to the gnucash-user list, as it's not related to this bug.
Moved to -user list for now.
Comment on attachment 148705 [details] [review] Patch to fix problems in comments 12 and 14 Uh oh. Applying this patch makes my gnucash installation crash during startup with a scheme error even earlier. Backtrace: In current input: 1: 0* (set! ACCOUNT-OPTIONS-SECTION (ACCOUNT-OPTIONS-SECTION)) <unnamed port>:1:1: In expression (set! ACCOUNT-OPTIONS-SECTION (ACCOUNT-OPTIONS-SECTION)): <unnamed port>:1:1: Unbound variable: ACCOUNT-OPTIONS-SECTION This is guile-1.6.8 and swig-1.3.36; the difference behaviour at your machine and mine must be caused by those two versions somehow.
With this patch I also get failed tests in src/engine: ERROR: Unbound variable: ACCOUNT-OPTIONS-SECTION FAIL: test-create-account ERROR: Unbound variable: ACCOUNT-OPTIONS-SECTION FAIL: test-scm-query ========================================== 2 of 22 tests failed Please report to gnucash-devel@gnucash.org ==========================================
This is really curious. This failure is coming in the initialization code generated by Swig as a result of processing engine.i. The three variables, BOOK_OPTIONS_NAME, ACCOUNT_OPTIONS_SECTION, and TRADING_ACCOUNTS_OPTION are handled in exactly the same way. There is a call to the SET_ENUM macro for each of them in the initialization function in engine.i and the second of these three calls (which is the 71st such call in SWIG_Init) is failing. The first one (for BOOK_OPTIONS_NAME), and the 69 previous ones, apparently work. Can you send me a copy of the swig-engine.c file generated when you built GnuCash with this problem? In the SWIG_Init function there should be a call to scm_c_define_gsubr to define ACCOUNT-OPTIONS-SECTION followed by a call to scm_c_eval_string that uses the definition. For some reason the second call fails. I have Guile 1.8.7 and Swig 1.3.40. That's quite a lot newer version of Guile, but this doesn't seem like something that should have changed recently.
I checked in a fix for the bug described in comment 19 in r18455. I still don't know what is causing the problem described in comments 26 and 27.
Fix for bug described in comment 19 confirmed.
(In reply to comment #29) > I checked in a fix for the bug described in comment 19 in r18455. I still > don't know what is causing the problem described in comments 26 and 27. Apparently this issue doesn't appear anymore on my machine, so you can probably close this bug.
(In reply to comment #28) > This is really curious. This failure is coming in the initialization code > generated by Swig as a result of processing engine.i. The three variables, > BOOK_OPTIONS_NAME, ACCOUNT_OPTIONS_SECTION, and TRADING_ACCOUNTS_OPTION are > handled in exactly the same way. There is a call to the SET_ENUM macro for > each of them in the initialization function in engine.i and the second of these > three calls (which is the 71st such call in SWIG_Init) is failing. The first > one (for BOOK_OPTIONS_NAME), and the 69 previous ones, apparently work. Can > you send me a copy of the swig-engine.c file generated when you built GnuCash > with this problem? In the SWIG_Init function there should be a call to > scm_c_define_gsubr to define ACCOUNT-OPTIONS-SECTION followed by a call to > scm_c_eval_string that uses the definition. For some reason the second call > fails. > > I have Guile 1.8.7 and Swig 1.3.40. That's quite a lot newer version of Guile, > but this doesn't seem like something that should have changed recently. I've run into this puzzling behavior in relation to the same file. find path/to/src/ -name '*.i' -exec touch '{}' \; or the like can help force swig to run, as will make maintainer-clean or compiling in a completely clean build directory. https://bugzilla.gnome.org/show_bug.cgi?id=612604
GnuCash bug tracking has moved to a new Bugzilla host. This bug has been copied to https://bugs.gnucash.org/show_bug.cgi?id=537476. Please update any external references or bookmarks.