Rationalize currency unit to “BTC” #6504

pull rnicoll wants to merge 1 commits into bitcoin:master from rnicoll:normalize-units changing 8 files +48 −40
  1. rnicoll commented at 7:39 pm on August 1, 2015: contributor

    Previously various user-facing strings have used inconsistent currency units “BTC”, “btc” and “bitcoins”. This adds a single constant and uses it for each reference to the currency unit.

    Also adds a description of the unit for –maxtxfee, and adds the missing “amount” field description to the (deprecated) move RPC command.

  2. theochino commented at 8:06 pm on August 1, 2015: none

    Hello,

    I would suggest using XBT instead of the BTx is the ISO prefix for Bhutan.

    Regard, Theo Chino, NYC (Participant of the foundation now defunct group for the ISO certification for Bitcoin)

    On Saturday, August 1, 2015, Ross Nicoll notifications@github.com wrote:

    Previously various user-facing strings have used inconsistent currency units “BTC”, “btc” and “bitcoins”. This adds a single constant and uses it for each reference to the currency unit.

    Also adds a description of the unit for –maxtxfee, and adds the missing

    “amount” field description to the (deprecated) move RPC command.

    You can view, comment on, or merge this pull request online at:

    #6504 Commit Summary

    • Rationalize currency unit to “BTC”

    File Changes

    Patch Links:

    — Reply to this email directly or view it on GitHub #6504.

  3. rnicoll commented at 8:15 pm on August 1, 2015: contributor
    I’ll leave others to discuss whether it should change, but this patch certainly makes it simpler by using a single constant (although QT wallet values would need to be updated independently)
  4. in src/init.cpp: in 253cc3fb03 outdated 
    341@@ -342,16 +342,17 @@ std::string HelpMessage(HelpMessageMode mode)
342     strUsage += HelpMessageOpt("-disablewallet", _("Do not load the wallet and disable wallet RPC calls"));
343     strUsage += HelpMessageOpt("-keypool=<n>", strprintf(_("Set key pool size to <n> (default: %u)"), 100));
344     if (showDebug)
345-        strUsage += HelpMessageOpt("-mintxfee=<amt>", strprintf("Fees (in BTC/Kb) smaller than this are considered zero fee for transaction creation (default: %s)",
    luke-jr commented at 10:18 pm on August 1, 2015:
    At least fix Kb to kb here at the same time..
  5. luke-jr commented at 10:19 pm on August 1, 2015: member
    @theochino XBT is not a well-defined denomination. Different parts of the community use it for different size bases.
  6. luke-jr commented at 10:20 pm on August 1, 2015: member
    Also, as to the concept of this PR itself: I don’t think we need to define a variable for every reused term. Just changing “btc” and “bitcoins” to “BTC” in literals should be sufficient.
  7. rnicoll force-pushed on Aug 2, 2015
  8. rnicoll commented at 9:23 am on August 2, 2015: contributor
    Fixed kilobyte units, and removed the constant string.
  9. rnicoll commented at 11:54 am on August 2, 2015: contributor
    Just realised these strings are all copied into qt/bitcoinstrings.cpp - are those extracted automatically and I shouldn’t touch them, or do they need updating too?
  10. laanwj added the label Refactoring on Aug 3, 2015
  11. laanwj commented at 8:17 am on August 3, 2015: member

    Concept ACK on moving the currency unit to a constant. Please do leave it as BTC. Changing it to XBT becomes trivial for those that insist on it, if it’s managed from one plcae.

    Just realised these strings are all copied into qt/bitcoinstrings.cpp - are those extracted automatically and I shouldn’t touch them, or do they need updating too?

    Don’t touch the file manually. Translation strings are extracted automatically by make translate’s tooling. This updates bitcoinstrings as well as the English ts_file.

  12. Diapolo commented at 2:11 pm on August 3, 2015: none
    This only seems to cover core code, any reason not to do this for the GUI part?
  13. rnicoll commented at 6:12 pm on August 3, 2015: contributor
    @Diapolo Couldn’t see any problems in the QT code, apart from the bitcoinstrings.cpp (which are generated), and that code uses its own copy of currency units generally. Do let me know if I’ve missed anything.
  14. laanwj commented at 6:46 pm on August 3, 2015: member
    Hm, you removed the parametrization?
  15. rnicoll commented at 6:56 pm on August 3, 2015: contributor
    Putting it back at the moment
  16. rnicoll commented at 7:14 pm on August 3, 2015: contributor
    Done - let me know if that’s okay and I’ll squash them
  17. in src/qt/bitcoinunits.cpp: in b4a7b7888f outdated 
    39@@ -40,9 +40,9 @@ QString BitcoinUnits::name(int unit)
40 {
41     switch(unit)
42     {
43-    case BTC: return QString("BTC");
44-    case mBTC: return QString("mBTC");
45-    case uBTC: return QString::fromUtf8("μBTC");
46+    case BTC: return QString(CURRENCY_UNIT.c_str());
47+    case mBTC: return QString(("m" + CURRENCY_UNIT).c_str());
    luke-jr commented at 7:30 pm on August 3, 2015:
    Did you test this? const char * + const char * is going to be a pointer to some undefined memory…
    rnicoll commented at 7:54 pm on August 3, 2015:
    It’s std::string + std::string I believe, and certainly seemed to work. Removing as per @laanwj ’s request anyway.
    laanwj commented at 8:15 pm on August 3, 2015:
    No longer relevant here, but for next time: you can use QString::fromStdString to convert a std::string to QString. No need for c_str.
  18. in src/wallet/rpcwallet.cpp: in b4a7b7888f outdated 
    2278@@ -2278,7 +2279,7 @@ UniValue listunspent(const UniValue& params, bool fHelp)
2279             "    \"address\" : \"address\",  (string) the bitcoin address\n"
2280             "    \"account\" : \"account\",  (string) DEPRECATED. The associated account, or \"\" for the default account\n"
2281             "    \"scriptPubKey\" : \"key\", (string) the script key\n"
2282-            "    \"amount\" : x.xxx,         (numeric) the transaction amount in btc\n"
2283+            "    \"amount\" : x.xxx,         (numeric) the transaction amount in BTC\n"
    luke-jr commented at 7:32 pm on August 3, 2015:
    Why not CURRENCY_UNIT here?
  19. in src/qt/bitcoinunits.cpp: in b4a7b7888f outdated 
    39@@ -40,9 +40,9 @@ QString BitcoinUnits::name(int unit)
40 {
41     switch(unit)
42     {
43-    case BTC: return QString("BTC");
44-    case mBTC: return QString("mBTC");
45-    case uBTC: return QString::fromUtf8("μBTC");
46+    case BTC: return QString(CURRENCY_UNIT.c_str());
    laanwj commented at 7:43 pm on August 3, 2015:
    Let’s leave this function as it is. So anyone changing the currency unit will have to change bitcoinunits as well, that’s OK IMO, that’s what it was introduced for. (This concatenation is weird, and will mismatch the constant names)
  20. in src/wallet/rpcwallet.cpp: in b4a7b7888f outdated 
    1686@@ -1686,7 +1687,7 @@ UniValue gettransaction(const UniValue& params, bool fHelp)
1687             "2. \"includeWatchonly\"    (bool, optional, default=false) Whether to include watchonly addresses in balance calculation and details[]\n"
1688             "\nResult:\n"
1689             "{\n"
1690-            "  \"amount\" : x.xxx,        (numeric) The transaction amount in btc\n"
1691+            "  \"amount\" : x.xxx,        (numeric) The transaction amount in BTC\n"
    laanwj commented at 7:45 pm on August 3, 2015:
    Seems a few more currency_unit are missing here
    rnicoll commented at 7:52 pm on August 3, 2015:
    This is what I get for trying to be too clever with search and replace… getting them now.
  21. rnicoll force-pushed on Aug 3, 2015
  22. rnicoll commented at 8:22 pm on August 3, 2015: contributor

    Fixed, and found a couple of uses of “bitcoin” as a unit too, which I’ve also fixed.

    Thanks for the tip re: QString

  23. Rationalize currency unit to "BTC" 
    Previously various user-facing strings have used inconsistent currency units "BTC",
"btc" and "bitcoins". This adds a single constant and uses it for each reference to
the currency unit.

Also adds a description of the unit for --maxtxfee, and adds the missing "amount"
field description to the (deprecated) move RPC command.
    9ca7857df7
  24. in src/rpcmining.cpp: in 2c9c69ab32 outdated 
    263@@ -264,7 +264,7 @@ UniValue getmininginfo(const UniValue& params, bool fHelp)
264 }
265 
266 
267-// NOTE: Unlike wallet RPC (which use BTC values), mining RPCs follow GBT (BIP 22) in using satoshi amounts
268+// NOTE: Unlike wallet RPC (which use " + CURRENCY_UNIT + " values), mining RPCs follow GBT (BIP 22) in using satoshi amounts
    laanwj commented at 8:58 pm on August 3, 2015:
    Comment :)
  25. laanwj commented at 9:58 pm on August 3, 2015: member
    Tested ACK (but please remove the runaway unit parametrization from the comment)
  26. rnicoll force-pushed on Aug 3, 2015
  27. rnicoll commented at 10:16 pm on August 3, 2015: contributor
    Fixed and squashed
  28. laanwj merged this on Aug 3, 2015
  29. laanwj closed this on Aug 3, 2015
  30. laanwj referenced this in commit b6fee6b7c7 on Aug 3, 2015
  31. theochino commented at 6:52 pm on September 25, 2015: none

    @luke-jr (On your August 1st comment.) - In 3 letters ISO codes, BTC will never be accepted (even it is used today) by the power to be.

    I do prefer the BTC version until I read everything there was on ISO4217 and participated in the foundation working group - https://en.wikipedia.org/wiki/ISO_4217

    Once I read it, X (Supranational) BT (Bitcoin) makes more scene than BTC which belong to Bhutan. I say let make it XBT and make it easy for everyone we want to use the Bitcoin.

    Just saying. XBT is free in the ISO code nomenclature. Why not claim it. People are confused enough as it is ….

  32. luke-jr commented at 8:37 pm on September 25, 2015: member
    “XBT” is fine as a symbol. My point is that it is not well-defined whether it represents 100,000,000 (BTC), 100,000 (mBTC), 100 (µBTC), or whatever else.
  33. theochino commented at 8:57 pm on September 25, 2015: none

    I see your point … leave it as BTC now and wait … maybe later 1 XBT = 1 µBTC. The problem, until we solve all the socials ills of the planet, is that will never be a one size fits all solution. Those with wealth will say 1 XBT = 100 BTC, and those on the other side will say 1 XBT = 1 mBTC.

    To avoid headaches, I use 1 BTC = 1 XBT, 100'000 mBTC = 100'000 mXBT, and 100 µBTC = 100 µXBT and leave the fixing of the social ill to the pope. ISO standard has a field for the decimal (and I believe it does go to 8) so it would be a “trivial” change to use 1 BTC = 1 XBT which makes it simpler in the code to replace.

    If the amount changes in the future, we’ll change it to something else when we cross that bridge. Anyway, it was a small “loaded” request ….

  34. sipa commented at 9:03 pm on September 25, 2015: member
    Unifying the currency names across the source code to a single constant is useful on itself, regardless of what people think that constant should be.
  35. str4d referenced this in commit 3d36684e5e on Feb 28, 2017
  36. str4d referenced this in commit eadc679056 on Feb 28, 2017
  37. str4d referenced this in commit 3c014397a9 on Mar 2, 2017
  38. zkbot referenced this in commit e0bef1de56 on Sep 18, 2017
  39. rnicoll deleted the branch on Mar 13, 2018
  40. DrahtBot locked this on Sep 8, 2021


rnicoll theochino luke-jr laanwj Diapolo sipa

Labels
Refactoring

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-19 09:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me