refactor: Remove confusing OutputType::CHANGE

MarcoFalke commented at 2:53 pm on June 27, 2020: member

OutputType::CHANGE_AUTO is problematic for several reasons:

An output that is not change must never be described by CHANGE_AUTO. Simply allowing that option makes the code confusing and review harder than it needs to be.
To make review even harder, CHANGE_AUTO requires -Wswitch to be disabled for OutputType

Fix both issues by removing CHANGE_AUTO and then enabling -Wswitch for OutputType

interfaces: Remove unused getDefaultChangeType fa2eb38352

MarcoFalke added the label Refactoring on Jun 27, 2020

MarcoFalke force-pushed on Jun 27, 2020

achow101 commented at 4:42 am on June 28, 2020: member

Concept ACK

DrahtBot commented at 3:47 pm on June 28, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#18354 (Use shared pointers only in validation interface by bvbfan)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in src/wallet/wallet.cpp:2660 in fa9cee2218 outdated

2658 {
2659     // If -changetype is specified, always use that change type.
2660-    if (change_type != OutputType::CHANGE_AUTO) {
2661-        return change_type;
2662+    if (change_type) {
2663+        return *change_type;

Sjors commented at 9:19 am on July 1, 2020:

I guess it’s non-trivial to expand our c++17 std::optional substitute with std::optional::value?

MarcoFalke commented at 11:57 am on July 1, 2020:

Sorry I don’t follow. https://en.cppreference.com/w/cpp/utility/optional/operator* exists in C++17

Sjors commented at 5:36 pm on July 1, 2020:

So you can use change_type->value() which looks prettier imo, and - more importantly - raises an exception if change_type isn’t set.

MarcoFalke commented at 8:22 pm on July 1, 2020:

change_type can never be unset in this line because the line is preceded by a check to see if it is set, so I’d rather not annotate this function to throw an exception when in reality it can not throw an exception.

in src/wallet/wallet.cpp:3834 in fa9cee2218 outdated

3830@@ -3831,7 +3831,7 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
3831         return nullptr;
3832     }
3833 
3834-    if (!gArgs.GetArg("-changetype", "").empty() && !ParseOutputType(gArgs.GetArg("-changetype", ""), walletInstance->m_default_change_type)) {
3835+    if (!gArgs.GetArg("-changetype", "").empty() && !ParseOutputType(gArgs.GetArg("-changetype", ""), *(walletInstance->m_default_change_type = OutputType{}))) {

Sjors commented at 9:24 am on July 1, 2020:

I’m not a fan of this assignment inside an if statement. See more (maybe overkill) verbose suggestion below.

MarcoFalke commented at 11:56 am on July 1, 2020:

ParseOutputType assigns a new value to the argument, so why can’t the argument itself not be initialized?

Note that with C++17, we can use walletInstance->m_default_change_type.emplace() instead.

0    if (!gArgs.GetArg("-changetype", "").empty() && !ParseOutputType(gArgs.GetArg("-changetype", ""), *walletInstance->m_default_change_type.emplace())) {

I am happy to change it to that once we switch to C++17

Sjors commented at 5:53 pm on July 1, 2020:

I don’t think it’s bad, but the line is hard to read. Another approach could be to initialise m_default_change_type in wallet.h; that doesn’t seem unreasonable for an Optional.

Update: no that doesn’t work. The key here is to initialize with nonsense so that you can dereference the optional and pass it into ParseOutputType. I find that yuck :-)

It seems cleaner to either pass a temporary variable to ParseOutputType or change it to handle an optional.

achow101 commented at 6:22 pm on July 1, 2020:

Agree that this is a bit hard to read.

MarcoFalke commented at 8:47 pm on July 1, 2020:

thx changed

in src/wallet/wallet.cpp:3843 in fa9cee2218 outdated

3830@@ -3831,7 +3831,7 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
3831         return nullptr;
3832     }
3833 
3834-    if (!gArgs.GetArg("-changetype", "").empty() && !ParseOutputType(gArgs.GetArg("-changetype", ""), walletInstance->m_default_change_type)) {
3835+    if (!gArgs.GetArg("-changetype", "").empty() && !ParseOutputType(gArgs.GetArg("-changetype", ""), *(walletInstance->m_default_change_type = OutputType{}))) {
3836         error = strprintf(_("Unknown change type '%s'"), gArgs.GetArg("-changetype", ""));
3837         return nullptr;
3838     }

Sjors commented at 9:34 am on July 1, 2020:

0     if (!gArgs.GetArg("-changetype", "").empty()) {
1        OutputType default_change_type;
2        if (ParseOutputType(gArgs.GetArg("-changetype", ""), default_change_type)) {
3            walletInstance->m_default_change_type.emplace(default_change_type);
4        } else {
5            error = strprintf(_("Unknown change type '%s'"), gArgs.GetArg("-changetype", ""));
6            return nullptr;
7        }
8    }

MarcoFalke commented at 8:47 pm on July 1, 2020:

thx changed

Sjors commented at 9:38 am on July 1, 2020: member

Concept ACK. fa37f9ae6d908a30affcfefda24e44ff3000207d looks mostly good

Needs wallet tag.

promag commented at 9:55 am on July 1, 2020: member

Code review ACK fa37f9ae6d908a30affcfefda24e44ff3000207d.

Tend to agree with @Sjors in #19396 (review).

MarcoFalke added the label Wallet on Jul 1, 2020

achow101 commented at 6:22 pm on July 1, 2020: member

ACK fa37f9ae6d908a30affcfefda24e44ff3000207d

MarcoFalke force-pushed on Jul 1, 2020

achow101 commented at 9:04 pm on July 1, 2020: member

ACK fabcf5fc9d126cba8d5637b1cf7a4e9c6047ce1b

in src/wallet/wallet.cpp:3838 in fa078530bc outdated

3839-    if (!gArgs.GetArg("-changetype", "").empty() && !ParseOutputType(gArgs.GetArg("-changetype", ""), walletInstance->m_default_change_type)) {
3840-        error = strprintf(_("Unknown change type '%s'"), gArgs.GetArg("-changetype", ""));
3841-        return nullptr;
3842+    if (!gArgs.GetArg("-changetype", "").empty()) {
3843+        walletInstance->m_default_change_type.emplace();
3844+        if (!ParseOutputType(gArgs.GetArg("-changetype", ""), *walletInstance->m_default_change_type)) {

promag commented at 9:31 pm on July 1, 2020:

fa078530bc3824d7d5a1cc4f39b617054b08d77e

nit, I’d rather ditch the above, declare a OutputType change_type in this scope, and assign to walletInstance->m_default_change_type if parse succeeds. Not really relevant but this approach changes m_default_change_type for no good reason if parse fails.

MarcoFalke commented at 10:05 pm on July 1, 2020:

It doesn’t matter here, but I changed it as a belt-and-suspenders measure

in src/wallet/rpcwallet.cpp:309 in fa078530bc outdated

305@@ -306,7 +306,7 @@ static UniValue getrawchangeaddress(const JSONRPCRequest& request)
306         throw JSONRPCError(RPC_WALLET_ERROR, "Error: This wallet has no available keys");
307     }
308 
309-    OutputType output_type = pwallet->m_default_change_type != OutputType::CHANGE_AUTO ? pwallet->m_default_change_type : pwallet->m_default_address_type;
310+    OutputType output_type = pwallet->m_default_change_type ? *pwallet->m_default_change_type : pwallet->m_default_address_type;

promag commented at 9:31 pm on July 1, 2020:

fa078530bc3824d7d5a1cc4f39b617054b08d77e

nit, I think you can use value_or() which is available in boost and std.

MarcoFalke commented at 10:04 pm on July 1, 2020:

Ah good point

in src/wallet/rpcwallet.cpp:2996 in fa078530bc outdated

2992@@ -2993,7 +2993,7 @@ void FundTransaction(CWallet* const pwallet, CMutableTransaction& tx, CAmount& f
2993             if (options.exists("changeAddress")) {
2994                 throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot specify both changeAddress and address_type options");
2995             }
2996-            coinControl.m_change_type = pwallet->m_default_change_type;
2997+            coinControl.m_change_type.emplace();

promag commented at 9:32 pm on July 1, 2020:

fa078530bc3824d7d5a1cc4f39b617054b08d77e

nit, same as #19396 (review), use temporary.

MarcoFalke commented at 10:05 pm on July 1, 2020:

changed for same reason

Remove confusing OutputType::CHANGE_AUTO faddad71f6

Enable Wswitch for OutputType fa927ff884

MarcoFalke force-pushed on Jul 1, 2020

MarcoFalke commented at 10:05 pm on July 1, 2020: member

Fixed @promag’s feedback

promag commented at 10:39 pm on July 1, 2020: member

Code review ACK fa927ff884ae373c676deed63180a8d238872cdc.

luke-jr commented at 2:02 am on July 2, 2020: member

Concept ACK

laanwj commented at 2:08 pm on July 2, 2020: member

Code review ACK fa927ff884ae373c676deed63180a8d238872cdc

laanwj merged this on Jul 2, 2020

laanwj closed this on Jul 2, 2020

MarcoFalke deleted the branch on Jul 2, 2020

Sjors commented at 5:00 pm on July 2, 2020: member

Post merge utACK

Fabcien referenced this in commit 1c09834d5f on Aug 30, 2021

Fabcien referenced this in commit a33b200ef6 on Aug 30, 2021

DrahtBot locked this on Feb 15, 2022

refactor: Remove confusing OutputType::CHANGE_AUTO #19396

Conflicts