[rpc] Return payment address on z_importkey #3754
Conversation
src/test/rpc_wallet_tests.cpp
Outdated
| BOOST_CHECK_NO_THROW(ret = CallRPC("z_validateaddress " + ret.get_str())); | ||
| ret = ret.get_obj(); | ||
| BOOST_CHECK_EQUAL(true, find_value(ret, "isvalid").get_bool()); | ||
| BOOST_CHECK_EQUAL(true, find_value(ret, "ismine").get_bool()); |
There was a problem hiding this comment.
When you support Sapling, please also extend the test to include importing a Sapling key.
src/wallet/rpcdump.cpp
Outdated
| @@ -586,6 +586,7 @@ UniValue z_importkey(const UniValue& params, bool fHelp) | |||
| "2. rescan (string, optional, default=\"whenkeyisnew\") Rescan the wallet for transactions - can be \"yes\", \"no\" or \"whenkeyisnew\"\n" | |||
| "3. startHeight (numeric, optional, default=0) Block height to start rescan from\n" | |||
| "\nNote: This call can take minutes to complete if rescan is true.\n" | |||
| "\nResult: Encoded payment address if possible, otherwise nothing.\n" | |||
src/wallet/rpcdump.cpp
Outdated
| auto sproutSpendingKey = boost::get<libzcash::SproutSpendingKey>(&spendingkey); | ||
| if (sproutSpendingKey != nullptr) { | ||
| return EncodePaymentAddress(sproutSpendingKey->address()); | ||
| } |
There was a problem hiding this comment.
Please also support Sapling keys/addresses.
|
Thanks for the feedback @daira. I pushed a commit adding sapling support as well. I'll squash the commits if the changes pass review. |
|
Moved and renamed |
There was a problem hiding this comment.
ut(ACK+cov). The only place I could see where this could potentially cause a regression would be if the key passed to the DefaultAddressFromSpendingKey visitor were an InvalidEncoding, but that shouldn't be able to happen because we've checked that IsValidSpendingKey(spendingkey) is true. Other reviewers should check my reasoning here.
daira
added
the
C-community-contribution
src/wallet/rpcdump.cpp
Outdated
| return NullUniValue; | ||
| auto addr = boost::apply_visitor( | ||
| libzcash::DefaultAddressFromSpendingKey{}, spendingkey); | ||
| return EncodePaymentAddress(addr); |
There was a problem hiding this comment.
I think I'd prefer to return a JSON object that contains a defaultAddress field set to this (or maybe primaryAddress field, since Sprout is supported; alternatively we could use address for Sprout and defaultAddress for Sapling, and have a type field like z_validateaddress has). That makes it clear that this is the default address in the case of Sapling, and also means that we can extend this RPC method in future with additional information if we wanted.
There was a problem hiding this comment.
I moved the address to a defaultAddress field as you suggested.
It looks like the best way to add the type field is to export DescribePaymentAddressVisitor, I suggest that I do that in a separate PR to limit the size of this one. What do you think?
|
☔ The latest upstream changes (presumably #4270) made this pull request unmergeable. Please resolve the merge conflicts. |
|
This has been folded into #4220, which I've requested be updated to include appropriate credit for work included from this PR. |
Fixes #3748