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

Replace "Bitcoin" with "Zcash" in strings #2150

Merged
merged 3 commits into from Oct 5, 2017
Merged

Replace "Bitcoin" with "Zcash" in strings #2150

merged 3 commits into from Oct 5, 2017

Conversation

brunoarueira
Copy link
Contributor

@brunoarueira brunoarueira commented Mar 3, 2017

Closes #1756

@daira daira self-assigned this Mar 3, 2017
@daira daira added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 3, 2017
daira
daira previously requested changes Mar 3, 2017
Copy link
Contributor

@daira daira left a comment

Choose a reason for hiding this comment

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

Looks good apart from the capitalization of ZEC and Zcash, and the indentation in the RPC help.

@@ -194,7 +194,7 @@ QT_TRANSLATE_NOOP("bitcoin-core", "Connect only to the specified node(s)"),
QT_TRANSLATE_NOOP("bitcoin-core", "Connect through SOCKS5 proxy"),
QT_TRANSLATE_NOOP("bitcoin-core", "Connect to a node to retrieve peer addresses, and disconnect"),
QT_TRANSLATE_NOOP("bitcoin-core", "Connection options:"),
QT_TRANSLATE_NOOP("bitcoin-core", "Copyright (C) 2009-%i The Bitcoin Core Developers"),
QT_TRANSLATE_NOOP("bitcoin-core", "Copyright (C) 2009-%i The Zcash Developers"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Bitcoin Core copyright notices can't be deleted. In this case it wouldn't matter very much because we're about to remove the QT wallet anyway, but can you remove either this change, or all the QT changes?

@@ -184,7 +184,7 @@ UniValue getrawmempool(const UniValue& params, bool fHelp)
"{ (json object)\n"
" \"transactionid\" : { (json object)\n"
" \"size\" : n, (numeric) transaction size in bytes\n"
" \"fee\" : n, (numeric) transaction fee in bitcoins\n"
" \"fee\" : n, (numeric) transaction fee in zec\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

s/zec/ZEC/

" \"reqSigs\" : n, (numeric) Number of required signatures\n"
" \"bestblock\" : \"hash\", (string) the block hash\n"
" \"confirmations\" : n, (numeric) The number of confirmations\n"
" \"value\" : x.xxx, (numeric) The transaction value in zec\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

ZEC (and similarly below)

" \"addresses\" : [ (array of string) array of bitcoin addresses\n"
" \"bitcoinaddress\" (string) bitcoin address\n"
" \"addresses\" : [ (array of string) array of zcash addresses\n"
" \"zcashaddress\" (string) zcash address\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

Zcash address

" \"type\" : \"pubkeyhash\", (string) The type, eg pubkeyhash\n"
" \"addresses\" : [ (array of string) array of bitcoin addresses\n"
" \"bitcoinaddress\" (string) bitcoin address\n"
" \"addresses\" : [ (array of string) array of zcash addresses\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

Zcash addresses (and similarly below)

" \"asm\" : \"code\", (string) \n"
" \"hex\" : \"hex\", (string) \n"
" \"reqSigs\" : n, (numeric) Number of required signatures\n"
" \"bestblock\" : \"hash\", (string) the block hash\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

If a line has four escaped quotes, the description should appear two characters further to the right than if it has only two escaped quotes (i.e. the escapes are nonprinting characters).

+ HelpExampleCli("sendtoaddress", "\"1M72Sfpbz1BPpXFHz9m3CdqATR44Jvaydd\" 0.1 \"donation\" \"seans outpost\"")
+ HelpExampleCli("sendtoaddress", "\"1M72Sfpbz1BPpXFHz9m3CdqATR44Jvaydd\" 0.1 \"\" \"\" true")
+ HelpExampleRpc("sendtoaddress", "\"1M72Sfpbz1BPpXFHz9m3CdqATR44Jvaydd\", 0.1, \"donation\", \"seans outpost\"")
+ HelpExampleCli("sendtoaddress", "\"tb4oHp2v54vfmdgQ3v3SNuQga8JKHTNi2a1\" 0.1")
Copy link
Contributor

Choose a reason for hiding this comment

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

This would start with t1.

@zkbot
Copy link
Contributor

zkbot commented Mar 3, 2017

☔ The latest upstream changes (presumably #2099) made this pull request unmergeable. Please resolve the merge conflicts.

@arcalinea
Copy link
Contributor

@daira I incorporated your comments, would love to get this merged in 1.0.8
Thanks for taking the initiative on this @brunoarueira !

@arcalinea
Copy link
Contributor

Rebased on master

@arcalinea arcalinea self-assigned this Mar 23, 2017
str4d
str4d previously requested changes Mar 23, 2017
Copy link
Contributor

@str4d str4d left a comment

Choose a reason for hiding this comment

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

There are many good changes in here, but I'd prefer that we first pull in bitcoin/bitcoin#6504 and then rebase this on top of that. We could do both in this same PR if desired. I will review more thoroughly afterwards.

src/init.cpp Outdated
<<<<<<< HEAD
=======

>>>>>>> Removes out bitcoin mention in favor for zcash
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't be here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ugh thought I got rid of this. Deleting.

Makes sense to pull in bitcoin/bitcoin#6504 first though

src/rpcmisc.cpp Outdated
@@ -61,8 +61,8 @@ UniValue getinfo(const UniValue& params, bool fHelp)
" \"keypoololdest\": xxxxxx, (numeric) the timestamp (seconds since GMT epoch) of the oldest pre-generated key in the key pool\n"
" \"keypoolsize\": xxxx, (numeric) how many new keys are pre-generated\n"
" \"unlocked_until\": ttt, (numeric) the timestamp in seconds since epoch (midnight Jan 1 1970 GMT) that the wallet is unlocked for transfers, or 0 if the wallet is locked\n"
" \"paytxfee\": x.xxxx, (numeric) the transaction fee set in btc/kb\n"
" \"relayfee\": x.xxxx, (numeric) minimum relay fee for non-free transactions in btc/kb\n"
" \"paytxfee\": x.xxxx, (numeric) the transaction fee set in ZEC/kb\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

These kinds of changes are better-handled IMHO by pulling in bitcoin/bitcoin#6504

@arcalinea
Copy link
Contributor

Sounds good, we can wait on bitcoin/bitcoin#6504 then.

We should also fix #2109 by making sure the help text displays zcash addresses. I'll open a separate PR for that

@daira daira dismissed their stale review March 26, 2017 03:07

Review obsoleted.

@daira daira added this to the 1.0.9 milestone Mar 26, 2017
@whyrusleeping
Copy link

Any update here? Getting a message talking about bitcoin when i run two zcashd's in regtest on the same computer (also, side note, zcashd's regtest uses the same port as bitcoin core)

zkbot added a commit that referenced this pull request Apr 17, 2017
Change help text examples to use Zcash addresses

Closes #1804 and #2109

Ensures command line help text addresses are Zcash addresses.

Didn't change the text strings that say bitcoin or btc, leaving that for #2150 to close

Did I get all the addresses?
@zkbot
Copy link
Contributor

zkbot commented Apr 17, 2017

☔ The latest upstream changes (presumably #2205) made this pull request unmergeable. Please resolve the merge conflicts.

@arcalinea
Copy link
Contributor

arcalinea commented May 4, 2017

I lost track of what this was blocking on. @str4d did that PR from upstream ever get pulled in? It would be really great to get this in 1.0.9 regardless, even if it seems like a marginal improvement for now, because it's a big improvement for usability. I'm working with the RPC interface a lot and this is a constant source of annoyance and confusion, and users keep opening new issues about it as well (#2336 filed a few days ago). Can we go ahead and merge this if I rebase again?

@str4d
Copy link
Contributor

str4d commented May 4, 2017

@arcalinea No - it's a big conflicting PR, which is why I was holding off pulling it in until others of my 0.12 PRs were merged, but I can prioritise doing so if this is being targeted for 1.0.9.

@nathan-at-least nathan-at-least removed this from the 1.0.9 milestone May 10, 2017
@nathan-at-least
Copy link
Contributor

Bumped from 1.0.9 due to Str4d's suggested PR dependency. Note: I didn't read the full backlog, but we're already over capacity for 1.0.9 and this is lower priority.

@nathan-at-least nathan-at-least modified the milestone: 1.0.9 May 19, 2017
@nathan-at-least
Copy link
Contributor

Bumped again. I apologize for the confusion. On Friday I was hunting for 'simple PRs' to pull into 1.0.9 to test some CI changes, and I had forgotten this was blocked on an upstream PR.

@daira
Copy link
Contributor

daira commented Aug 2, 2017

See also #2548.

@daira daira added this to Proposed in Release planning Aug 3, 2017
@str4d
Copy link
Contributor

str4d commented Aug 3, 2017

I have opened #2564 with the upstream PR; this PR should be rebased on master once that merges.

@nathan-at-least nathan-at-least moved this from Proposed to Misc in Release planning Aug 15, 2017
@str4d str4d moved this from Misc to 1.0.13: Cleanups in Release planning Aug 21, 2017
@bitcartel
Copy link
Contributor

I've rebased (but can't push to the PR to update). What's the magic git/github incantation to achieve this?

@str4d
Copy link
Contributor

str4d commented Oct 4, 2017

@bitcartel git push -f (append your local name for the OP's remote repo if your branch isn't tracking it).

@bitcartel
Copy link
Contributor

@str4d Thanks, the -f did the trick.

Since #2564 has closed and been merged, this PR is ready for review and merge.

@bitcartel bitcartel added this to the 1.0.13 milestone Oct 4, 2017
brunoarueira and others added 3 commits October 4, 2017 17:05
Bitcoin Core => Zcash
bitcoin address => Zcash address
bitcoinaddress => zcashaddress

Closes #1756
@bitcartel bitcartel dismissed str4d’s stale review October 5, 2017 00:08

Upstream pulled in and rebased

@bitcartel
Copy link
Contributor

@str4d Commits tidied up as discussed.

Copy link
Contributor

@str4d str4d left a comment

Choose a reason for hiding this comment

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

utACK

" \"addresses\" : [ (array of string) array of bitcoin addresses\n"
" \"bitcoinaddress\" (string) bitcoin address\n"
" \"addresses\" : [ (array of string) array of Zcash addresses\n"
" \"zcashaddress\" (string) Zcash address\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'll assume that these whitespace changes actually make the output line up.

@str4d
Copy link
Contributor

str4d commented Oct 5, 2017

@zkbot r+

@zkbot
Copy link
Contributor

zkbot commented Oct 5, 2017

📌 Commit 6de8501 has been approved by str4d

@zkbot
Copy link
Contributor

zkbot commented Oct 5, 2017

⌛ Testing commit 6de8501 with merge ebe0cb3e05725fc891caa13a91147f8baa5c7a8e...

@str4d
Copy link
Contributor

str4d commented Oct 5, 2017

@zkbot r-

@str4d
Copy link
Contributor

str4d commented Oct 5, 2017

@zkbot force

@str4d
Copy link
Contributor

str4d commented Oct 5, 2017

@zkbot clean

@str4d str4d changed the title Removes out btc mention in favor for zec Replace "Bitcoin" with "Zcash" in strings Oct 5, 2017
@str4d
Copy link
Contributor

str4d commented Oct 5, 2017

@zkbot r+

@zkbot
Copy link
Contributor

zkbot commented Oct 5, 2017

📌 Commit 6de8501 has been approved by str4d

@zkbot
Copy link
Contributor

zkbot commented Oct 5, 2017

⌛ Testing commit 6de8501 with merge ab28fc4...

zkbot added a commit that referenced this pull request Oct 5, 2017
Replace "Bitcoin" with "Zcash" in strings

Closes #1756
@zkbot
Copy link
Contributor

zkbot commented Oct 5, 2017

☀️ Test successful - pr-merge
Approved by: str4d
Pushing ab28fc4 to master...

@zkbot zkbot merged commit 6de8501 into zcash:master Oct 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
No open projects
Continuous Improvement
  
Awaiting triage
Development

Successfully merging this pull request may close these issues.

None yet

8 participants