From fe384eeb1f5bf3d94a98f6b0f6915e4da47ec411 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 13 Jan 2022 18:45:40 +0000 Subject: [PATCH 1/5] wallet: Implement `z_getnewaccount` Closes zcash/zcash#5178. --- qa/pull-tester/rpc-tests.py | 1 + qa/rpc-tests/wallet_accounts.py | 31 +++++++++++++++++++++++++++++++ src/wallet/rpcwallet.cpp | 23 ++++++++++++++--------- src/wallet/walletdb.h | 1 + 4 files changed, 47 insertions(+), 9 deletions(-) create mode 100755 qa/rpc-tests/wallet_accounts.py diff --git a/qa/pull-tester/rpc-tests.py b/qa/pull-tester/rpc-tests.py index 1d39e86ae..e00f1640b 100755 --- a/qa/pull-tester/rpc-tests.py +++ b/qa/pull-tester/rpc-tests.py @@ -64,6 +64,7 @@ BASE_SCRIPTS= [ 'p2p-fullblocktest.py', # vv Tests less than 30s vv 'wallet_1941.py', + 'wallet_accounts.py', 'wallet_addresses.py', 'wallet_anchorfork.py', 'wallet_changeindicator.py', diff --git a/qa/rpc-tests/wallet_accounts.py b/qa/rpc-tests/wallet_accounts.py new file mode 100755 index 000000000..b31cb47ce --- /dev/null +++ b/qa/rpc-tests/wallet_accounts.py @@ -0,0 +1,31 @@ +#!/usr/bin/env python3 +# Copyright (c) 2022 The Zcash developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or https://www.opensource.org/licenses/mit-license.php . + +from test_framework.test_framework import BitcoinTestFramework +from test_framework.util import ( + assert_equal, + start_nodes, +) + +# Test wallet accounts behaviour +class WalletAccountsTest(BitcoinTestFramework): + def setup_nodes(self): + return start_nodes(self.num_nodes, self.options.tmpdir, [[ + '-experimentalfeatures', + '-orchardwallet', + ]] * self.num_nodes) + + def run_test(self): + # With a new wallet, the first account will be 0. + account0 = self.nodes[0].z_getnewaccount() + assert_equal(account0['account'], 0) + + # The next account will be 1. + account1 = self.nodes[0].z_getnewaccount() + assert_equal(account1['account'], 1) + + +if __name__ == '__main__': + WalletAccountsTest().main() diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index c1a87477b..79e4573e3 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -16,6 +16,7 @@ #include "proof_verifier.h" #include "rpc/server.h" #include "timedata.h" +#include "tinyformat.h" #include "transaction_builder.h" #include "util.h" #include "util/match.h" @@ -3016,18 +3017,15 @@ UniValue z_getnewaccount(const UniValue& params, bool fHelp) if (fHelp || params.size() > 0) throw runtime_error( "z_getnewaccount\n" - "\nPrepares and returns a new account, and its corresponding default address.\n" + "\nPrepares and returns a new account.\n" "\nAccounts are numbered starting from zero; this RPC method selects the next" "\navailable sequential account number within the UA-compatible HD seed phrase.\n" - "\nThe account will be prepared with spending keys for the best and second-best" - "\nshielded pools, and the transparent pool.\n" "\nEach new account is a separate group of funds within the wallet, and adds an" - "\nadditional performance cost to wallet scanning. If you want a new address" - "\nfor an existing account, use the z_getaddressforaccount RPC method.\n" + "\nadditional performance cost to wallet scanning.\n" + "\nUse the z_getaddressforaccount RPC method to obtain addresses for an account.\n" "\nResult:\n" "{\n" " \"account\": n, (numeric) the new account number\n" - " \"unifiedaddress\" (string) The default address for this account\n" "}\n" "\nExamples:\n" + HelpExampleCli("z_getnewaccount", "") @@ -3037,10 +3035,17 @@ UniValue z_getnewaccount(const UniValue& params, bool fHelp) if (!fExperimentalOrchardWallet) { throw JSONRPCError(RPC_WALLET_ENCRYPTION_FAILED, "Error: the Orchard wallet experimental extensions are disabled."); } - int64_t account = 999999; // TODO placeholder + + LOCK(pwalletMain->cs_wallet); + + EnsureWalletIsUnlocked(); + + // Generate the new account. + auto skNew = pwalletMain->GenerateNewUnifiedSpendingKey(); + const auto& account = skNew.second; + UniValue result(UniValue::VOBJ); - result.pushKV("account", account); - result.pushKV("unifiedaddress", "TODO"); + result.pushKV("account", (uint64_t)account); return result; } diff --git a/src/wallet/walletdb.h b/src/wallet/walletdb.h index 8fc72d01f..36ed07fa5 100644 --- a/src/wallet/walletdb.h +++ b/src/wallet/walletdb.h @@ -101,6 +101,7 @@ public: } void IncrementAccountCounter() { + // TODO: We should check for overflow somewhere and handle it. accountCounter += 1; } From 8617622e0d41a22e8e3056bf6412f389e2e13576 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 13 Jan 2022 22:43:21 +0000 Subject: [PATCH 2/5] wallet: Implement `z_getaddressforaccount` Closes zcash/zcash#5180. --- qa/rpc-tests/wallet_accounts.py | 27 ++++++++ src/test/util_tests.cpp | 26 ++++++++ src/uint256.cpp | 7 ++ src/utilstrencodings.cpp | 35 ++++++++++ src/utilstrencodings.h | 4 ++ src/wallet/rpcwallet.cpp | 113 ++++++++++++++++++++++++++------ 6 files changed, 191 insertions(+), 21 deletions(-) diff --git a/qa/rpc-tests/wallet_accounts.py b/qa/rpc-tests/wallet_accounts.py index b31cb47ce..e3eb299ca 100755 --- a/qa/rpc-tests/wallet_accounts.py +++ b/qa/rpc-tests/wallet_accounts.py @@ -3,9 +3,11 @@ # Distributed under the MIT software license, see the accompanying # file COPYING or https://www.opensource.org/licenses/mit-license.php . +from test_framework.authproxy import JSONRPCException from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_equal, + assert_raises_message, start_nodes, ) @@ -26,6 +28,31 @@ class WalletAccountsTest(BitcoinTestFramework): account1 = self.nodes[0].z_getnewaccount() assert_equal(account1['account'], 1) + # Generate the first address for account 0. + addr0 = self.nodes[0].z_getaddressforaccount(0) + assert_equal(addr0['account'], 0) + assert_equal(set(addr0['pools']), set(['transparent', 'sapling'])) + ua0 = addr0['unifiedaddress'] + + # We pick mnemonic phrases to ensure that we can always generate the default + # address in account 0; this is however not necessarily at diversifier index 0. + # We should be able to generate it directly and get the exact same data. + j = addr0['diversifier_index'] + assert_equal(self.nodes[0].z_getaddressforaccount(0, j), addr0) + if j > 0: + # We should get an error if we generate the address at diversifier index 0. + assert_raises_message( + JSONRPCException, + 'no address at diversifier index 0', + self.nodes[0].z_getaddressforaccount, 0, 0) + + # The first address for account 1 is different to account 0. + addr1 = self.nodes[0].z_getaddressforaccount(1) + assert_equal(addr1['account'], 1) + assert_equal(set(addr1['pools']), set(['transparent', 'sapling'])) + ua1 = addr1['unifiedaddress'] + assert(ua0 != ua1) + if __name__ == '__main__': WalletAccountsTest().main() diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp index 5834b3e0f..28ee6b97d 100644 --- a/src/test/util_tests.cpp +++ b/src/test/util_tests.cpp @@ -595,4 +595,30 @@ BOOST_AUTO_TEST_CASE(test_ParseArbitraryInt) BOOST_CHECK_EQUAL((*v)[21], 0x10); } +BOOST_AUTO_TEST_CASE(test_ArbitraryIntStr) +{ + BOOST_CHECK_EQUAL(ArbitraryIntStr({}), "0"); + BOOST_CHECK_EQUAL(ArbitraryIntStr({0}), "0"); + BOOST_CHECK_EQUAL(ArbitraryIntStr({0, 0}), "0"); + BOOST_CHECK_EQUAL(ArbitraryIntStr({0, 0, 0}), "0"); + BOOST_CHECK_EQUAL(ArbitraryIntStr({0, 0, 0, 0}), "0"); + + BOOST_CHECK_EQUAL(ArbitraryIntStr({1}), "1"); + BOOST_CHECK_EQUAL(ArbitraryIntStr({2}), "2"); + BOOST_CHECK_EQUAL(ArbitraryIntStr({10}), "10"); + BOOST_CHECK_EQUAL(ArbitraryIntStr({100}), "100"); + + BOOST_CHECK_EQUAL(ArbitraryIntStr({0xff}), "255"); + BOOST_CHECK_EQUAL(ArbitraryIntStr({0x00, 0x01}), "256"); + BOOST_CHECK_EQUAL(ArbitraryIntStr({0xff, 0x01}), "511"); + BOOST_CHECK_EQUAL(ArbitraryIntStr({0x00, 0x02}), "512"); + + BOOST_CHECK_EQUAL( + ArbitraryIntStr({0xff, 0xff, 0xff, 0xff}), + "4294967295"); + BOOST_CHECK_EQUAL( + ArbitraryIntStr({0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff}), + "309485009821345068724781055"); +} + BOOST_AUTO_TEST_SUITE_END() diff --git a/src/uint256.cpp b/src/uint256.cpp index 8c27e0570..1e9558ac8 100644 --- a/src/uint256.cpp +++ b/src/uint256.cpp @@ -64,6 +64,13 @@ std::string base_blob::ToString() const return (GetHex()); } +// Explicit instantiations for base_blob<88> +template base_blob<88>::base_blob(const std::vector&); +template std::string base_blob<88>::GetHex() const; +template std::string base_blob<88>::ToString() const; +template void base_blob<88>::SetHex(const char*); +template void base_blob<88>::SetHex(const std::string&); + // Explicit instantiations for base_blob<160> template base_blob<160>::base_blob(const std::vector&); template std::string base_blob<160>::GetHex() const; diff --git a/src/utilstrencodings.cpp b/src/utilstrencodings.cpp index 5665d4c9d..215e2aa37 100644 --- a/src/utilstrencodings.cpp +++ b/src/utilstrencodings.cpp @@ -528,3 +528,38 @@ std::optional> ParseArbitraryInt(const std::string& num_str } return result; } + +std::string ArbitraryIntStr(std::vector bytes) +{ + // Only serialize up to the most significant non-zero byte. + size_t end = bytes.size(); + for (; end > 0 && bytes[end - 1] == 0; --end) {} + + std::string result; + while (end > 0) { + // "Divide" bytes by 10. + uint16_t rem = 0; + for (int i = end - 1; i >= 0; --i) { + uint16_t tmp = rem * 256 + bytes[i]; + rem = tmp % 10; + auto b = tmp / 10; + assert(b < 256); + bytes[i] = b; + } + + // Write out the remainder as the next lowest digit. + result = tfm::format("%d%s", rem, result); + + // If we've moved all the bits out of the MSB, drop it. + if (bytes[end - 1] == 0) { + end--; + } + } + + // Handle the all-zero bytes case. + if (result.empty()) { + return "0"; + } else { + return result; + } +} diff --git a/src/utilstrencodings.h b/src/utilstrencodings.h index 04073db6f..22cd6b67f 100644 --- a/src/utilstrencodings.h +++ b/src/utilstrencodings.h @@ -168,5 +168,9 @@ bool ConvertBits(const O& outfn, I it, I end) { } std::optional> ParseArbitraryInt(const std::string& s); +/** + * Serializes the given little-endian byte iterator to a decimal string. + */ +std::string ArbitraryIntStr(std::vector i); #endif // BITCOIN_UTILSTRENCODINGS_H diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 79e4573e3..cb5c55804 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -21,12 +21,14 @@ #include "util.h" #include "util/match.h" #include "utilmoneystr.h" +#include "utilstrencodings.h" #include "wallet.h" #include "walletdb.h" #include "primitives/transaction.h" #include "zcbenchmarks.h" #include "script/interpreter.h" #include "zcash/Address.hpp" +#include "zcash/address/zip32.h" #include "utiltime.h" #include "asyncrpcoperation.h" @@ -3085,13 +3087,18 @@ UniValue z_getaddressforaccount(const UniValue& params, bool fHelp) if (!fExperimentalOrchardWallet) { throw JSONRPCError(RPC_WALLET_ENCRYPTION_FAILED, "Error: the Orchard wallet experimental extensions are disabled."); } - int64_t account = params[0].get_int64(); - if (account < 0 || account >= ZCASH_LEGACY_ACCOUNT) { + + LOCK(pwalletMain->cs_wallet); + + int64_t accountInt = params[0].get_int64(); + if (accountInt < 0 || accountInt >= ZCASH_LEGACY_ACCOUNT) { throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid account number, must be 0 <= account <= (2^31)-2."); } - // TODO: Check that the account is known to the wallet. - std::vector parsed_diversifier_index; + libzcash::AccountId account = accountInt; + + std::optional j = std::nullopt; if (params.size() >= 2) { + // TODO: Handle '*' if (params[1].getType() != UniValue::VNUM) { throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid diversifier index, must be an unsigned integer."); } @@ -3099,36 +3106,100 @@ UniValue z_getaddressforaccount(const UniValue& params, bool fHelp) if (!parsed_diversifier_index_opt.has_value()) { throw JSONRPCError(RPC_INVALID_PARAMETER, "diversifier index must be a decimal integer."); } - parsed_diversifier_index = parsed_diversifier_index_opt.value(); + auto parsed_diversifier_index = parsed_diversifier_index_opt.value(); if (parsed_diversifier_index.size() > ZC_DIVERSIFIER_SIZE) { throw JSONRPCError(RPC_INVALID_PARAMETER, "diversifier index is too large."); } - } else { - // TODO get next unused diversifier index from wallet + // Extend the byte array to the correct length for diversifier_index_t. + parsed_diversifier_index.resize(ZC_DIVERSIFIER_SIZE); + j = libzcash::diversifier_index_t(parsed_diversifier_index); } - // TODO: - // diversifier_t diversifier{}; - // std::copy(parsed_diversifier_index.begin(), parsed_diversifier_index.end(), diversifier.begin()); - UniValue pools(UniValue::VARR); + + std::set receivers; if (params.size() >= 3) { - pools = params[2].get_array(); + const auto& pools = params[2].get_array(); for (unsigned int i = 0; i < pools.size(); i++) { const std::string& p = pools[i].get_str(); - if (!(p == "transparent" || p == "sapling" || p == "orchard")) { - throw JSONRPCError(RPC_INVALID_PARAMETER, "pool arguments must be \"transparent\", \"sapling\", or \"orchard\""); + if (p == "transparent") { + receivers.insert(ReceiverType::P2PKH); + } else if (p == "sapling") { + receivers.insert(ReceiverType::Sapling); + } else { + throw JSONRPCError(RPC_INVALID_PARAMETER, "pool arguments must be \"transparent\", or \"sapling\""); } } } else { - // default is all - pools.push_back("transparent"); - pools.push_back("sapling"); - pools.push_back("orchard"); + // Default is the best and second-best shielded pools, and the transparent pool. + receivers = {ReceiverType::P2PKH, ReceiverType::Sapling}; } + + EnsureWalletIsUnlocked(); + + // Generate the first UA for this account, using the best and next-best shielded pools + // and the transparent pool. + auto res = pwalletMain->GenerateUnifiedAddress(account, receivers, j); + UniValue result(UniValue::VOBJ); - result.pushKV("account", account); - result.pushKV("diversifier_index", params[1].write()); + result.pushKV("account", (uint64_t)account); + + std::visit(match { + [&](std::pair addr) { + result.pushKV("unifiedaddress", KeyIO(Params()).EncodePaymentAddress(addr.first)); + UniValue j; + j.setNumStr(ArbitraryIntStr(std::vector(addr.second.begin(), addr.second.end()))); + result.pushKV("diversifier_index", j); + }, + [&](AddressGenerationError err) { + std::string strErr; + switch (err) { + case AddressGenerationError::NoSuchAccount: + strErr = tfm::format("Error: account %d has not been generated by z_getnewaccount.", account); + break; + case AddressGenerationError::ExistingAddressMismatch: + strErr = tfm::format( + "Error: address at diversifier index %s was already generated with different receiver types.", + params[1].getValStr()); + break; + case AddressGenerationError::NoAddressForDiversifier: + strErr = tfm::format( + "Error: no address at diversifier index %s.", + ArbitraryIntStr(std::vector(j.value().begin(), j.value().end()))); + break; + case AddressGenerationError::InvalidTransparentChildIndex: + strErr = tfm::format( + "Error: diversifier index %s cannot generate an address with a transparent receiver.", + ArbitraryIntStr(std::vector(j.value().begin(), j.value().end()))); + break; + default: + // By construction, we will not see these errors here: + // - InvalidReceiverTypes + // - WalletEncrypted + // + // If we see these, the user either has generated many addresses, or + // was very unlucky with their mnemonic phrase generation: + // - DiversifierSpaceExhausted + strErr = tfm::format("Error: ran out of diversifier indices. Generate a new account with z_getnewaccount"); + } + throw JSONRPCError(RPC_WALLET_ERROR, strErr); + }, + }, res); + + UniValue pools(UniValue::VARR); + for (const auto& receiver : receivers) { + switch (receiver) { + case ReceiverType::P2PKH: + pools.push_back("transparent"); + break; + case ReceiverType::Sapling: + pools.push_back("sapling"); + break; + default: + // Unreachable + assert(false); + } + } result.pushKV("pools", pools); - result.pushKV("unifiedaddress", "TODO"); + return result; } From baaa3c4ac02afec3ca21843ab08cc0fcc1c2a190 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Fri, 14 Jan 2022 00:56:33 +0000 Subject: [PATCH 3/5] wallet: Implement `z_listunifiedreceivers` Closes zcash/zcash#5181. --- qa/rpc-tests/wallet_accounts.py | 8 ++++++++ src/wallet/rpcwallet.cpp | 29 +++++++++++++++++++++++++---- 2 files changed, 33 insertions(+), 4 deletions(-) diff --git a/qa/rpc-tests/wallet_accounts.py b/qa/rpc-tests/wallet_accounts.py index e3eb299ca..e5a594f6d 100755 --- a/qa/rpc-tests/wallet_accounts.py +++ b/qa/rpc-tests/wallet_accounts.py @@ -19,6 +19,10 @@ class WalletAccountsTest(BitcoinTestFramework): '-orchardwallet', ]] * self.num_nodes) + def check_receiver_types(self, ua, expected): + actual = self.nodes[0].z_listunifiedreceivers(ua) + assert_equal(set(expected), set(actual)) + def run_test(self): # With a new wallet, the first account will be 0. account0 = self.nodes[0].z_getnewaccount() @@ -53,6 +57,10 @@ class WalletAccountsTest(BitcoinTestFramework): ua1 = addr1['unifiedaddress'] assert(ua0 != ua1) + # The UA contains the expected receiver kinds. + self.check_receiver_types(ua0, ['transparent', 'sapling']) + self.check_receiver_types(ua1, ['transparent', 'sapling']) + if __name__ == '__main__': WalletAccountsTest().main() diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index cb5c55804..7f174860b 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3285,11 +3285,32 @@ UniValue z_listunifiedreceivers(const UniValue& params, bool fHelp) if (!fExperimentalOrchardWallet) { throw JSONRPCError(RPC_WALLET_ENCRYPTION_FAILED, "Error: the Orchard wallet experimental extensions are disabled."); } - std::string ua = params[0].get_str(); + + KeyIO keyIO(Params()); + auto decoded = keyIO.DecodePaymentAddress(params[0].get_str()); + if (!decoded.has_value()) { + throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid address"); + } + if (!std::holds_alternative(decoded.value())) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "Address is not a unified address"); + } + auto ua = std::get(decoded.value()); + UniValue result(UniValue::VOBJ); - result.pushKV("transparent", "TODO"); - result.pushKV("sapling", "TODO"); - result.pushKV("orchard", "TODO " + ua); + for (const auto& receiver : ua) { + std::visit(match { + [&](const libzcash::SaplingPaymentAddress& addr) { + result.pushKV("sapling", keyIO.EncodePaymentAddress(addr)); + }, + [&](const CScriptID& addr) { + result.pushKV("transparent", keyIO.EncodePaymentAddress(addr)); + }, + [&](const CKeyID& addr) { + result.pushKV("transparent", keyIO.EncodePaymentAddress(addr)); + }, + [](auto rest) {}, + }, receiver); + } return result; } From ff0e9f6b95842cb079a2ccd81119ac7415705905 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Fri, 14 Jan 2022 15:34:46 +0000 Subject: [PATCH 4/5] wallet: Show UAs owned by the wallet in `z_viewtransaction` Part of zcash/zcash#5186. --- qa/rpc-tests/wallet_accounts.py | 34 +++++++++++++++++++++++++++++++++ src/wallet/rpcwallet.cpp | 24 +++++++++++++++++++++-- 2 files changed, 56 insertions(+), 2 deletions(-) diff --git a/qa/rpc-tests/wallet_accounts.py b/qa/rpc-tests/wallet_accounts.py index e5a594f6d..779af5c32 100755 --- a/qa/rpc-tests/wallet_accounts.py +++ b/qa/rpc-tests/wallet_accounts.py @@ -8,9 +8,13 @@ from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_equal, assert_raises_message, + get_coinbase_address, start_nodes, + wait_and_assert_operationid_status, ) +from decimal import Decimal + # Test wallet accounts behaviour class WalletAccountsTest(BitcoinTestFramework): def setup_nodes(self): @@ -61,6 +65,36 @@ class WalletAccountsTest(BitcoinTestFramework): self.check_receiver_types(ua0, ['transparent', 'sapling']) self.check_receiver_types(ua1, ['transparent', 'sapling']) + # Manually send funds to one of the receivers in the UA. + # TODO: Once z_sendmany supports UAs, receive to the UA instead of the receiver. + sapling0 = self.nodes[0].z_listunifiedreceivers(ua0)['sapling'] + recipients = [{'address': sapling0, 'amount': Decimal('10')}] + opid = self.nodes[0].z_sendmany(get_coinbase_address(self.nodes[0]), recipients, 1, 0) + txid = wait_and_assert_operationid_status(self.nodes[0], opid) + + # The wallet should detect the new note as belonging to the UA. + tx_details = self.nodes[0].z_viewtransaction(txid) + assert_equal(len(tx_details['outputs']), 1) + assert_equal(tx_details['outputs'][0]['type'], 'sapling') + assert_equal(tx_details['outputs'][0]['address'], ua0) + + self.sync_all() + self.nodes[2].generate(1) + self.sync_all() + + # Manually send funds from the UA receiver. + # TODO: Once z_sendmany supports UAs, send from the UA instead of the receiver. + node1sapling = self.nodes[1].z_getnewaddress('sapling') + recipients = [{'address': node1sapling, 'amount': Decimal('1')}] + opid = self.nodes[0].z_sendmany(sapling0, recipients, 1, 0) + txid = wait_and_assert_operationid_status(self.nodes[0], opid) + + # The wallet should detect the spent note as belonging to the UA. + tx_details = self.nodes[0].z_viewtransaction(txid) + assert_equal(len(tx_details['spends']), 1) + assert_equal(tx_details['spends'][0]['type'], 'sapling') + assert_equal(tx_details['spends'][0]['address'], ua0) + if __name__ == '__main__': WalletAccountsTest().main() diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 7f174860b..1ae42b106 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3959,12 +3959,22 @@ UniValue z_viewtransaction(const UniValue& params, bool fHelp) assert(pwalletMain->GetSaplingFullViewingKey(wtxPrev.mapSaplingNoteData.at(op).ivk, extfvk)); ovks.insert(extfvk.fvk.ovk); + // If the note belongs to a Sapling address that is part of an account in the + // wallet, show the corresponding Unified Address. + std::string address; + const auto ua = pwalletMain->GetUnifiedForReceiver(pa); + if (ua.has_value()) { + address = keyIO.EncodePaymentAddress(ua.value()); + } else { + address = keyIO.EncodePaymentAddress(pa); + } + UniValue entry(UniValue::VOBJ); entry.pushKV("type", ADDR_TYPE_SAPLING); entry.pushKV("spend", (int)i); entry.pushKV("txidPrev", op.hash.GetHex()); entry.pushKV("outputPrev", (int)op.n); - entry.pushKV("address", keyIO.EncodePaymentAddress(pa)); + entry.pushKV("address", address); entry.pushKV("value", ValueFromAmount(notePt.value())); entry.pushKV("valueZat", notePt.value()); spends.push_back(entry); @@ -4002,11 +4012,21 @@ UniValue z_viewtransaction(const UniValue& params, bool fHelp) } auto memo = notePt.memo(); + // If the note belongs to a Sapling address that is part of an account in the + // wallet, show the corresponding Unified Address. + std::string address; + const auto ua = pwalletMain->GetUnifiedForReceiver(pa); + if (ua.has_value()) { + address = keyIO.EncodePaymentAddress(ua.value()); + } else { + address = keyIO.EncodePaymentAddress(pa); + } + UniValue entry(UniValue::VOBJ); entry.pushKV("type", ADDR_TYPE_SAPLING); entry.pushKV("output", (int)op.n); entry.pushKV("outgoing", isOutgoing); - entry.pushKV("address", keyIO.EncodePaymentAddress(pa)); + entry.pushKV("address", address); entry.pushKV("value", ValueFromAmount(notePt.value())); entry.pushKV("valueZat", notePt.value()); addMemo(entry, memo); From 96c9333b7498a86c99ddc0ed78f0101e5e9dc683 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Tue, 18 Jan 2022 20:02:13 +0000 Subject: [PATCH 5/5] wallet: Reverse order of arguments to z_getaddressforaccount We now use the empty array of pools to indicate that the default pools should be used when providing a diversifier index parameter, instead of needing a default sentinel value for the diversifier index parameter (which was previously suggested as '*' which would have caused issues for defining a consistent type for that parameter). --- qa/rpc-tests/wallet_accounts.py | 4 +-- src/wallet/rpcwallet.cpp | 60 ++++++++++++++++----------------- 2 files changed, 32 insertions(+), 32 deletions(-) diff --git a/qa/rpc-tests/wallet_accounts.py b/qa/rpc-tests/wallet_accounts.py index 779af5c32..f693599c0 100755 --- a/qa/rpc-tests/wallet_accounts.py +++ b/qa/rpc-tests/wallet_accounts.py @@ -46,13 +46,13 @@ class WalletAccountsTest(BitcoinTestFramework): # address in account 0; this is however not necessarily at diversifier index 0. # We should be able to generate it directly and get the exact same data. j = addr0['diversifier_index'] - assert_equal(self.nodes[0].z_getaddressforaccount(0, j), addr0) + assert_equal(self.nodes[0].z_getaddressforaccount(0, [], j), addr0) if j > 0: # We should get an error if we generate the address at diversifier index 0. assert_raises_message( JSONRPCException, 'no address at diversifier index 0', - self.nodes[0].z_getaddressforaccount, 0, 0) + self.nodes[0].z_getaddressforaccount, 0, [], 0) # The first address for account 1 is different to account 0. addr1 = self.nodes[0].z_getaddressforaccount(1) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 1ae42b106..01461035f 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3057,13 +3057,13 @@ UniValue z_getaddressforaccount(const UniValue& params, bool fHelp) return NullUniValue; if (fHelp || params.size() < 1 || params.size() > 3) throw runtime_error( - "z_getaddressforaccount account ( diversifier_index [\"pool\", ...] )\n" + "z_getaddressforaccount account ( [\"pool\", ...] diversifier_index )\n" "\nFor the given account number, derives a Unified Address in accordance" "\nwith the remaining arguments:\n" - "\n- If no list of pools is given, the best and second-best shielded pools," - "\n along with the transparent pool, will be used." - "\n- If no diversifier index is given (or the string \"*\"), the next unused" - "\n index (that is valid for the list of pools) will be selected.\n" + "\n- If no list of pools is given (or the empty list \"[]\"), the best and" + "\n second-best shielded pools, along with the transparent pool, will be used." + "\n- If no diversifier index is given, the next unused index (that is valid" + "\n for the list of pools) will be selected.\n" "\nThe account number must have been previously generated by a call to the" "\nz_getnewaccount RPC method.\n" "\nOnce a Unified Address has been derived at a specific diversifier index," @@ -3079,8 +3079,8 @@ UniValue z_getaddressforaccount(const UniValue& params, bool fHelp) "}\n" "\nExamples:\n" + HelpExampleCli("z_getaddressforaccount", "4") - + HelpExampleCli("z_getaddressforaccount", "4 1") - + HelpExampleCli("z_getaddressforaccount", "4 1 '[\"transparent\",\"sapling\",\"orchard\"]'") + + HelpExampleCli("z_getaddressforaccount", "4 '[]' 1") + + HelpExampleCli("z_getaddressforaccount", "4 '[\"transparent\",\"sapling\",\"orchard\"]' 1") + HelpExampleRpc("z_getaddressforaccount", "4") ); @@ -3096,28 +3096,9 @@ UniValue z_getaddressforaccount(const UniValue& params, bool fHelp) } libzcash::AccountId account = accountInt; - std::optional j = std::nullopt; - if (params.size() >= 2) { - // TODO: Handle '*' - if (params[1].getType() != UniValue::VNUM) { - throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid diversifier index, must be an unsigned integer."); - } - auto parsed_diversifier_index_opt = ParseArbitraryInt(params[1].getValStr()); - if (!parsed_diversifier_index_opt.has_value()) { - throw JSONRPCError(RPC_INVALID_PARAMETER, "diversifier index must be a decimal integer."); - } - auto parsed_diversifier_index = parsed_diversifier_index_opt.value(); - if (parsed_diversifier_index.size() > ZC_DIVERSIFIER_SIZE) { - throw JSONRPCError(RPC_INVALID_PARAMETER, "diversifier index is too large."); - } - // Extend the byte array to the correct length for diversifier_index_t. - parsed_diversifier_index.resize(ZC_DIVERSIFIER_SIZE); - j = libzcash::diversifier_index_t(parsed_diversifier_index); - } - std::set receivers; - if (params.size() >= 3) { - const auto& pools = params[2].get_array(); + if (params.size() >= 2) { + const auto& pools = params[1].get_array(); for (unsigned int i = 0; i < pools.size(); i++) { const std::string& p = pools[i].get_str(); if (p == "transparent") { @@ -3128,11 +3109,30 @@ UniValue z_getaddressforaccount(const UniValue& params, bool fHelp) throw JSONRPCError(RPC_INVALID_PARAMETER, "pool arguments must be \"transparent\", or \"sapling\""); } } - } else { + } + if (receivers.empty()) { // Default is the best and second-best shielded pools, and the transparent pool. receivers = {ReceiverType::P2PKH, ReceiverType::Sapling}; } + std::optional j = std::nullopt; + if (params.size() >= 3) { + if (params[2].getType() != UniValue::VNUM) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid diversifier index, must be an unsigned integer."); + } + auto parsed_diversifier_index_opt = ParseArbitraryInt(params[2].getValStr()); + if (!parsed_diversifier_index_opt.has_value()) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "diversifier index must be a decimal integer."); + } + auto parsed_diversifier_index = parsed_diversifier_index_opt.value(); + if (parsed_diversifier_index.size() > ZC_DIVERSIFIER_SIZE) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "diversifier index is too large."); + } + // Extend the byte array to the correct length for diversifier_index_t. + parsed_diversifier_index.resize(ZC_DIVERSIFIER_SIZE); + j = libzcash::diversifier_index_t(parsed_diversifier_index); + } + EnsureWalletIsUnlocked(); // Generate the first UA for this account, using the best and next-best shielded pools @@ -3158,7 +3158,7 @@ UniValue z_getaddressforaccount(const UniValue& params, bool fHelp) case AddressGenerationError::ExistingAddressMismatch: strErr = tfm::format( "Error: address at diversifier index %s was already generated with different receiver types.", - params[1].getValStr()); + params[2].getValStr()); break; case AddressGenerationError::NoAddressForDiversifier: strErr = tfm::format(