Skip to content

Conversation

Sjors
Copy link
Member

@Sjors Sjors commented Jan 18, 2018

Making it clear that these parameters can be overridden by individual wallet commands.

Requesting 0.16 tag, as it's probably not worth changing otherwise.

This makes grateful use of the fact that @sipa used an underscore for address_type and change_type RPC arguments.

@maflcko maflcko added this to the 0.16.0 milestone Jan 18, 2018
@maflcko maflcko added the Docs label Jan 18, 2018
Making it clear that these parameters can be overridden by individual wallet commands.

-BEGIN VERIFY SCRIPT-
sed -i 's/addresstype/defaultaddresstype/g' doc/*.md src/wallet/*.cpp test/functional/*.py
sed -i 's/changetype/defaultchangetype/g' doc/*.md src/wallet/*.cpp test/functional/*.py
-END VERIFY SCRIPT-
@Sjors Sjors force-pushed the 2018/01/defaultaddresstype branch from a71d991 to e47af54 Compare January 18, 2018 13:42
@Sjors
Copy link
Member Author

Sjors commented Jan 18, 2018

@MarcoFalke this changes more than just docs, probably needs a wallet and RPC label.

Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

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

utACK e47af54. I think this is nice to make it clear that these options won't somehow cause the bitcoin node to dictate an address type.

@@ -16,8 +16,8 @@
std::string GetWalletHelpString(bool showDebug)
{
std::string strUsage = HelpMessageGroup(_("Wallet options:"));
strUsage += HelpMessageOpt("-addresstype", strprintf(_("What type of addresses to use (\"legacy\", \"p2sh-segwit\", or \"bech32\", default: \"%s\")"), FormatOutputType(OUTPUT_TYPE_DEFAULT)));
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rather improve the description.

BTW, currently RPC help is very clear:

"2. \"address_type\" (string, optional) The address type to use. Options are \"legacy\", \"p2sh-segwit\", and \"bech32\". Default is set by -addresstype.\n"

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jan 18, 2018
Reasons for rename:

  1) Default value is `<datadir>/wallets` so calling it `-walletsdir` instead
     of `-walletdir` would be more internally consistent

  2) Directory can contain more than one wallet so plural makes more sense

  3) This makes it harder to confuse -walletdir option with -wallet option
     if we store wallets in their own directories as proposed in
     bitcoin#11466 (comment) and
     implemented in bitcoin#12216

-BEGIN VERIFY SCRIPT-
git grep -l walletdir | xargs sed -i s/walletdir/walletsdir/g
-END VERIFY SCRIPT-
@laanwj laanwj removed this from the 0.16.0 milestone Jan 18, 2018
@laanwj
Copy link
Member

laanwj commented Jan 18, 2018

Removing this from 0.16 milestone - during the IRC meeting it was found that many options apply to defaults, so adding default here would be inconsistent. Would be better to improve the help message instead.

@jonasschnelli
Copy link
Contributor

Should be closed then because changing the arguments after a release is a no-go.

@maflcko maflcko closed this Jan 18, 2018
@maflcko maflcko added this to the 0.16.0 milestone Jan 18, 2018
@Sjors Sjors deleted the 2018/01/defaultaddresstype branch January 19, 2018 07:53
@Sjors
Copy link
Member Author

Sjors commented Jan 19, 2018

Agree with the above, as well as @laanwj's point "shorter option names are easier to remember/type". The behavior of these parameters did lead to some confusion during code review, but with the proper docs hopefully it will be fine for users.

@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.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants