Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rationalize currency unit to "BTC" #6504

Merged
merged 1 commit into from Aug 3, 2015
Merged

Conversation

rnicoll
Copy link
Contributor

@rnicoll rnicoll commented Aug 1, 2015

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.

@theochino
Copy link

theochino commented Aug 1, 2015

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.

@rnicoll
Copy link
Contributor Author

rnicoll commented Aug 1, 2015

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)

@@ -342,16 +342,17 @@ std::string HelpMessage(HelpMessageMode mode)
strUsage += HelpMessageOpt("-disablewallet", _("Do not load the wallet and disable wallet RPC calls"));
strUsage += HelpMessageOpt("-keypool=<n>", strprintf(_("Set key pool size to <n> (default: %u)"), 100));
if (showDebug)
strUsage += HelpMessageOpt("-mintxfee=<amt>", strprintf("Fees (in BTC/Kb) smaller than this are considered zero fee for transaction creation (default: %s)",
Copy link
Member

@luke-jr luke-jr Aug 1, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least fix Kb to kb here at the same time..

@luke-jr
Copy link
Member

luke-jr commented Aug 1, 2015

@theochino XBT is not a well-defined denomination. Different parts of the community use it for different size bases.

@luke-jr
Copy link
Member

luke-jr commented Aug 1, 2015

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.

@rnicoll
Copy link
Contributor Author

rnicoll commented Aug 2, 2015

Fixed kilobyte units, and removed the constant string.

@rnicoll
Copy link
Contributor Author

rnicoll commented Aug 2, 2015

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?

@laanwj
Copy link
Member

laanwj commented Aug 3, 2015

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.

@Diapolo
Copy link

Diapolo commented Aug 3, 2015

This only seems to cover core code, any reason not to do this for the GUI part?

@rnicoll
Copy link
Contributor Author

rnicoll commented Aug 3, 2015

@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.

@laanwj
Copy link
Member

laanwj commented Aug 3, 2015

Hm, you removed the parametrization?

@rnicoll
Copy link
Contributor Author

rnicoll commented Aug 3, 2015

Putting it back at the moment

@rnicoll
Copy link
Contributor Author

rnicoll commented Aug 3, 2015

Done - let me know if that's okay and I'll squash them

case mBTC: return QString("mBTC");
case uBTC: return QString::fromUtf8("μBTC");
case BTC: return QString(CURRENCY_UNIT.c_str());
case mBTC: return QString(("m" + CURRENCY_UNIT).c_str());
Copy link
Member

@luke-jr luke-jr Aug 3, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you test this? const char * + const char * is going to be a pointer to some undefined memory...

Copy link
Contributor Author

@rnicoll rnicoll Aug 3, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's std::string + std::string I believe, and certainly seemed to work. Removing as per @laanwj 's request anyway.

Copy link
Member

@laanwj laanwj Aug 3, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@rnicoll
Copy link
Contributor Author

rnicoll commented Aug 3, 2015

Fixed, and found a couple of uses of "bitcoin" as a unit too, which I've also fixed.

Thanks for the tip re: QString

@@ -264,7 +264,7 @@ UniValue getmininginfo(const UniValue& params, bool fHelp)
}


// NOTE: Unlike wallet RPC (which use BTC values), mining RPCs follow GBT (BIP 22) in using satoshi amounts
// NOTE: Unlike wallet RPC (which use " + CURRENCY_UNIT + " values), mining RPCs follow GBT (BIP 22) in using satoshi amounts
Copy link
Member

@laanwj laanwj Aug 3, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment :)

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.
@laanwj
Copy link
Member

laanwj commented Aug 3, 2015

Tested ACK (but please remove the runaway unit parametrization from the comment)

@rnicoll
Copy link
Contributor Author

rnicoll commented Aug 3, 2015

Fixed and squashed

@laanwj laanwj merged commit 9ca7857 into bitcoin:master Aug 3, 2015
laanwj added a commit that referenced this pull request Aug 3, 2015
9ca7857 Rationalize currency unit to "BTC" (Ross Nicoll)
@theochino
Copy link

theochino commented Sep 25, 2015

@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 ....

@luke-jr
Copy link
Member

luke-jr commented Sep 25, 2015

"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.

@theochino
Copy link

theochino commented Sep 25, 2015

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 ....

@sipa
Copy link
Member

sipa commented Sep 25, 2015

str4d added a commit to str4d/zcash that referenced this pull request Feb 28, 2017
str4d added a commit to str4d/zcash that referenced this pull request Feb 28, 2017
str4d added a commit to str4d/zcash that referenced this pull request Mar 2, 2017
zkbot added a commit to zcash/zcash that referenced this pull request Sep 18, 2017
Rationalize currency unit to "ZEC"

Cherry-picked from the upstream PR bitcoin/bitcoin#6504

Part of #2074
@rnicoll rnicoll deleted the normalize-units branch Mar 13, 2018
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants