Skip to content

Minor error message fix #1160

Merged
merged 1 commit into from May 5, 2012

5 participants

@petertodd
Bitcoin member

No description provided.

@sipa sipa and 1 other commented on an outdated diff Apr 28, 2012
src/bitcoinrpc.cpp
@@ -999,7 +999,9 @@ Value addmultisigaddress(const Array& params, bool fHelp)
strAccount = AccountFromValue(params[2]);
// Gather public keys
- if ((nRequired < 1) || ((int)keys.size() < nRequired))
+ if (nRequired < 1)
+ throw runtime_error("must require at least one key");
@sipa
Bitcoin member
sipa added a note Apr 28, 2012

"must require" sounds strange

@petertodd
Bitcoin member
petertodd added a note Apr 28, 2012

See my update below for a better version. Also a small grammer fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Diapolo Diapolo and 2 others commented on an outdated diff Apr 29, 2012
src/bitcoinrpc.cpp
@@ -986,7 +986,7 @@ Value addmultisigaddress(const Array& params, bool fHelp)
if (fHelp || params.size() < 2 || params.size() > 3)
{
string msg = "addmultisigaddress <nrequired> <'[\"key\",\"key\"]'> [account]\n"
- "Add a nrequired-to-sign multisignature address to the wallet\"\n"
+ "Add an n-required-to-sign multisignature address to the wallet\"\n"
@Diapolo
Diapolo added a note Apr 29, 2012

Should read "Add a n-required...", as "an" goes before words that begin with vowels. Perhaps the devs intended to use the spelling "nrequired"?

@petertodd
Bitcoin member
petertodd added a note Apr 29, 2012

Ha, yeah you're right. Elementary school was a long time ago...

The term nrequired is always capitalized as nRequired elsewhere in the code, so I think it's meant to read as two words.

@sipa
Bitcoin member
sipa added a note Apr 30, 2012

n is just the prefix given to variable names that hold numbers

@petertodd
Bitcoin member
petertodd added a note Apr 30, 2012

Ahh, Hungarian.

Since it's user facing though, I'd still use the english writing convention of n-required for the rpc docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jgarzik
Bitcoin member
jgarzik commented May 1, 2012

NAK for "Grammer", ACK for "fixed non-sensical error message"

@petertodd
Bitcoin member

Sure, I'll delete "grammer" if that's the consensus.

@sipa
Bitcoin member
sipa commented May 2, 2012

Not sure whether it's intentional or not, but it's called "grammar" in English.

@petertodd
Bitcoin member

I need to follow this up for the pull request "Me and my buddies fixed them spelling."

@Diapolo
Diapolo commented May 3, 2012

NACK, as he included an already merged commit.

@sipa
Bitcoin member
sipa commented May 3, 2012

Oh, thanks for noticing.

@Diapolo
Diapolo commented May 3, 2012

@retep You have to rebase to current master, so that only your commit goes into this pull-req.
e.g. rebase origin upstream

@petertodd petertodd Fixed non-sensical error message
Previously trying to create a multisig address that required less than
one signature would output something like the following:

"wrong number of keys(got 1, need at least 0)"
86c47a5
@petertodd
Bitcoin member

@Diapolo Thanks. I think I fixed it.

Sorry, this is the first time I've used github pull requests before.

@Diapolo
Diapolo commented May 4, 2012

Looks good now :) and I had a hard time learning Git, too ... don't worry.

@gmaxwell gmaxwell merged commit 5b8def7 into bitcoin:master May 5, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.