rpc: simplify getaddressinfo labels, deprecate previous behavior

jonatack · jonatack · commit 60aba1f2f115 · 2020-01-03T19:46:20.000+01:00
- change the value returned in the RPC getaddressinfo `labels` field to an array
  of label name strings

- deprecate the previous behavior of returning a JSON hash structure containing
  label `name` and address `purpose` key/value pairs

- update the relevant tests
diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
@@ -3744,18 +3744,20 @@ UniValue getaddressinfo(const JSONRPCRequest& request)
             "                                                         getaddressinfo output fields for the embedded address, excluding metadata (timestamp, hdkeypath,\n"
             "                                                         hdseedid) and relation to the wallet (ismine, iswatchonly).\n"
             "  \"iscompressed\" : true|false,        (boolean, optional) If the pubkey is compressed.\n"
-            "  \"label\" :  \"label\"                  (string) The label associated with the address. Defaults to \"\". Equivalent to the name field in the labels array.\n"
+            "  \"label\" :  \"label\"                  (string) The label associated with the address. Defaults to \"\". Equivalent to the label name in the labels array below.\n"
             "  \"timestamp\" : timestamp,            (number, optional) The creation time of the key, if available, expressed in " + UNIX_EPOCH_TIME + ".\n"
             "  \"hdkeypath\" : \"keypath\"             (string, optional) The HD keypath, if the key is HD and available.\n"
             "  \"hdseedid\" : \"<hash160>\"            (string, optional) The Hash160 of the HD seed.\n"
             "  \"hdmasterfingerprint\" : \"<hash160>\" (string, optional) The fingerprint of the master key.\n"
-            "  \"labels\"                            (object) An array of labels associated with the address. Currently limited to one label but returned\n"
+            "  \"labels\"                            (json object) An array of labels associated with the address. Currently limited to one label but returned\n"
             "                                               as an array to keep the API stable if multiple labels are enabled in the future.\n"
             "    [\n"
+            "      \"label name\" (string) The label name. Defaults to \"\". Equivalent to the label field above.\n\n"
+            "      DEPRECATED, will be removed in 0.21. To re-enable, launch bitcoind with `-deprecatedrpc=labelspurpose`:\n"
             "      { (json object of label data)\n"
-            "        \"name\": \"label name\" (string) The label name. Defaults to \"\". Equivalent to the label field above.\n"
-            "        \"purpose\": \"purpose\" (string) The purpose of the associated address (send or receive).\n"
-            "      },...\n"
+            "        \"name\" : \"label name\" (string) The label name. Defaults to \"\". Equivalent to the label field above.\n"
+            "        \"purpose\" : \"purpose\" (string) The purpose of the associated address (send or receive).\n"
+            "      }\n"
             "    ]\n"
             "}\n"
                 },
@@ -3769,7 +3771,6 @@ UniValue getaddressinfo(const JSONRPCRequest& request)
 
     UniValue ret(UniValue::VOBJ);
     CTxDestination dest = DecodeDestination(request.params[0].get_str());
-
     // Make sure the destination is valid
     if (!IsValidDestination(dest)) {
         throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid address");
@@ -3800,7 +3801,7 @@ UniValue getaddressinfo(const JSONRPCRequest& request)
 
     // Return label field if existing. Currently only one label can be
     // associated with an address, so the label should be equivalent to the
-    // value of the name key/value pair in the labels hash array below.
+    // value of the name key/value pair in the labels array below.
     if (pwallet->mapAddressBook.count(dest)) {
         ret.pushKV("label", pwallet->mapAddressBook[dest].name);
     }
@@ -3819,15 +3820,22 @@ UniValue getaddressinfo(const JSONRPCRequest& request)
         }
     }
 
-    // Return a labels array containing a hash of key/value pairs for the label
-    // name and address purpose. The name value is equivalent to the label field
-    // above. Currently only one label can be associated with an address, but we
-    // return an array so the API remains stable if we allow multiple labels to
-    // be associated with an address in the future.
+    // Return a `labels` array containing the label associated with the address,
+    // equivalent to the `label` field above. Currently only one label can be
+    // associated with an address, but we return an array so the API remains
+    // stable if we allow multiple labels to be associated with an address in
+    // the future.
+    //
+    // DEPRECATED: The previous behavior of returning an array containing a JSON
+    // object of `name` and `purpose` key/value pairs has been deprecated.
     UniValue labels(UniValue::VARR);
     std::map<CTxDestination, CAddressBookData>::iterator mi = pwallet->mapAddressBook.find(dest);
     if (mi != pwallet->mapAddressBook.end()) {
-        labels.push_back(AddressBookDataToJSON(mi->second, true));
+        if (pwallet->chain().rpcEnableDeprecated("labelspurpose")) {
+            labels.push_back(AddressBookDataToJSON(mi->second, true));
+        } else {
+            labels.push_back(mi->second.name);
+        }
     }
     ret.pushKV("labels", std::move(labels));
 
diff --git a/test/functional/test_framework/wallet_util.py b/test/functional/test_framework/wallet_util.py
@@ -88,11 +88,6 @@ def get_multisig(node):
                     p2sh_p2wsh_script=CScript([OP_HASH160, witness_script, OP_EQUAL]).hex(),
                     p2sh_p2wsh_addr=script_to_p2sh_p2wsh(script_code))
 
-def labels_value(name="", purpose="receive"):
-    """Generate a getaddressinfo labels array from a name and purpose.
-    Often used as the value of a labels kwarg for calling test_address below."""
-    return [{"name": name, "purpose": purpose}]
-
 def test_address(node, address, **kwargs):
     """Get address info for `address` and test whether the returned values are as expected."""
     addr_info = node.getaddressinfo(address)
diff --git a/test/functional/wallet_basic.py b/test/functional/wallet_basic.py
@@ -15,10 +15,7 @@
     connect_nodes,
     wait_until,
 )
-from test_framework.wallet_util import (
-    labels_value,
-    test_address,
-)
+from test_framework.wallet_util import test_address
 
 
 class WalletTest(BitcoinTestFramework):
@@ -395,7 +392,7 @@ def run_test(self):
             for label in [u'рыба', u'𝅘𝅥𝅯']:
                 addr = self.nodes[0].getnewaddress()
                 self.nodes[0].setlabel(addr, label)
-                test_address(self.nodes[0], addr, label=label, labels=labels_value(name=label))
+                test_address(self.nodes[0], addr, label=label, labels=[label])
                 assert label in self.nodes[0].listlabels()
         self.nodes[0].rpc.ensure_ascii = True  # restore to default
 
diff --git a/test/functional/wallet_import_with_label.py b/test/functional/wallet_import_with_label.py
@@ -11,10 +11,7 @@
 """
 
 from test_framework.test_framework import BitcoinTestFramework
-from test_framework.wallet_util import (
-    labels_value,
-    test_address,
-)
+from test_framework.wallet_util import test_address
 
 
 class ImportWithLabel(BitcoinTestFramework):
@@ -40,19 +37,15 @@ def run_test(self):
                      iswatchonly=True,
                      ismine=False,
                      label=label,
-                     labels=labels_value(name=label))
+                     labels=[label])
 
         self.log.info(
             "Import the watch-only address's private key without a "
             "label and the address should keep its label."
         )
         priv_key = self.nodes[0].dumpprivkey(address)
         self.nodes[1].importprivkey(priv_key)
-
-        test_address(self.nodes[1],
-                     address,
-                     label=label,
-                     labels=labels_value(name=label))
+        test_address(self.nodes[1], address, label=label, labels=[label])
 
         self.log.info(
             "Test importaddress without label and importprivkey with label."
@@ -65,7 +58,7 @@ def run_test(self):
                      iswatchonly=True,
                      ismine=False,
                      label="",
-                     labels=labels_value())
+                     labels=[""])
 
         self.log.info(
             "Import the watch-only address's private key with a "
@@ -75,10 +68,7 @@ def run_test(self):
         label2 = "Test Label 2"
         self.nodes[1].importprivkey(priv_key2, label2)
 
-        test_address(self.nodes[1],
-                     address2,
-                     label=label2,
-                     labels=labels_value(name=label2))
+        test_address(self.nodes[1], address2, label=label2, labels=[label2])
 
         self.log.info("Test importaddress with label and importprivkey with label.")
         self.log.info("Import a watch-only address with a label.")
@@ -90,7 +80,7 @@ def run_test(self):
                      iswatchonly=True,
                      ismine=False,
                      label=label3_addr,
-                     labels=labels_value(name=label3_addr))
+                     labels=[label3_addr])
 
         self.log.info(
             "Import the watch-only address's private key with a "
@@ -100,10 +90,7 @@ def run_test(self):
         label3_priv = "Test Label 3 for importprivkey"
         self.nodes[1].importprivkey(priv_key3, label3_priv)
 
-        test_address(self.nodes[1],
-                     address3,
-                     label=label3_priv,
-                     labels=labels_value(name=label3_priv))
+        test_address(self.nodes[1], address3, label=label3_priv, labels=[label3_priv])
 
         self.log.info(
             "Test importprivkey won't label new dests with the same "
@@ -118,7 +105,7 @@ def run_test(self):
                      iswatchonly=True,
                      ismine=False,
                      label=label4_addr,
-                     labels=labels_value(name=label4_addr),
+                     labels=[label4_addr],
                      embedded=None)
 
         self.log.info(
@@ -131,15 +118,9 @@ def run_test(self):
         self.nodes[1].importprivkey(priv_key4)
         embedded_addr = self.nodes[1].getaddressinfo(address4)['embedded']['address']
 
-        test_address(self.nodes[1],
-                     embedded_addr,
-                     label="",
-                     labels=labels_value())
+        test_address(self.nodes[1], embedded_addr, label="", labels=[""])
 
-        test_address(self.nodes[1],
-                     address4,
-                     label=label4_addr,
-                     labels=labels_value(name=label4_addr))
+        test_address(self.nodes[1], address4, label=label4_addr, labels=[label4_addr])
 
         self.stop_nodes()
 
diff --git a/test/functional/wallet_importmulti.py b/test/functional/wallet_importmulti.py
@@ -29,7 +29,6 @@
 from test_framework.wallet_util import (
     get_key,
     get_multisig,
-    labels_value,
     test_address,
 )
 
@@ -571,7 +570,7 @@ def run_test(self):
                      solvable=True,
                      ismine=True,
                      label=p2sh_p2wpkh_label,
-                     labels=labels_value(name=p2sh_p2wpkh_label))
+                     labels=[p2sh_p2wpkh_label])
 
         # Test ranged descriptor fails if range is not specified
         xpriv = "tprv8ZgxMBicQKsPeuVhWwi6wuMQGfPKi9Li5GtX35jVNknACgqe3CY4g5xgkfDDJcmtF7o1QnxWDRYw4H5P26PXq7sbcUkEqeR4fg3Kxp2tigg"
@@ -643,7 +642,7 @@ def run_test(self):
                      solvable=True,
                      ismine=False,
                      label=p2pkh_label,
-                     labels=labels_value(name=p2pkh_label))
+                     labels=[p2pkh_label])
 
         # Test import fails if both desc and scriptPubKey are provided
         key = get_key(self.nodes[0])
diff --git a/test/functional/wallet_labels.py b/test/functional/wallet_labels.py
@@ -13,10 +13,8 @@
 
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import assert_equal, assert_raises_rpc_error
-from test_framework.wallet_util import (
-    labels_value,
-    test_address,
-)
+from test_framework.wallet_util import test_address
+
 
 class WalletLabelsTest(BitcoinTestFramework):
     def set_test_params(self):
@@ -157,12 +155,7 @@ def verify(self, node):
         if self.receive_address is not None:
             assert self.receive_address in self.addresses
         for address in self.addresses:
-            test_address(
-                node,
-                address,
-                label=self.name,
-                labels=labels_value(name=self.name, purpose=self.purpose[address])
-             )
+            test_address(node, address, label=self.name, labels=[self.name])
         assert self.name in node.listlabels()
         assert_equal(
             node.getaddressesbylabel(self.name),
diff --git a/test/functional/wallet_listreceivedby.py b/test/functional/wallet_listreceivedby.py
@@ -11,10 +11,7 @@
     assert_equal,
     assert_raises_rpc_error,
 )
-from test_framework.wallet_util import (
-    labels_value,
-    test_address,
-)
+from test_framework.wallet_util import test_address
 
 
 class ReceivedByTest(BitcoinTestFramework):
@@ -131,7 +128,7 @@ def run_test(self):
         # set pre-state
         label = ''
         address = self.nodes[1].getnewaddress()
-        test_address(self.nodes[1], address, label=label, labels=labels_value(name=label))
+        test_address(self.nodes[1], address, label=label, labels=[label])
         received_by_label_json = [r for r in self.nodes[1].listreceivedbylabel() if r["label"] == label][0]
         balance_by_label = self.nodes[1].getreceivedbylabel(label)
 
