New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Sapling support to z_getnewaddress and z_listaddresses #3429

Merged
merged 8 commits into from Aug 1, 2018

Conversation

@str4d
Copy link
Contributor

str4d commented Jul 30, 2018

Closes #3217.

str4d added some commits Jul 9, 2018

Raise the 90-character limit on Bech32 encodings
Regtest addresses are 91 characters, and ZIP 32's Bech32 encoding will
be significantly longer.

@str4d str4d added this to the v2.0.0 milestone Jul 30, 2018

@str4d str4d requested review from bitcartel and daira Jul 30, 2018

@str4d str4d added this to In Review in Zcashd Team Jul 30, 2018

@str4d str4d referenced this pull request Jul 30, 2018

Closed

Add Sapling support to wallet RPCs #3063

14 of 14 tasks complete

@Eirik0 Eirik0 self-requested a review Jul 30, 2018

@Eirik0

This comment has been minimized.

Copy link
Contributor

Eirik0 commented Jul 30, 2018

I am getting a "Permission denied" error when I run ./qa/pull-tester/rpc-tests.sh wallet_addresses.py. Are the permissions for these files something we are supposed to be updating manually? I have run in to this before.

@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Jul 30, 2018

@Eirik0 try removing the cache folder inside your zcash directory and then run the test again. I am guessing that the permissions on the cache folder (which this test uses) might have been corrupted at some point.

@Eirik0

This comment has been minimized.

Copy link
Contributor

Eirik0 commented Jul 30, 2018

It was actually the wallet_addresses.py file. I updated that manually and that fixed the problem. I have seen this before when I have created new rpc tests and done the chmod before pushing. I wasn't sure if this was something that was supposed to be happening automatically.

@ebfull

ebfull approved these changes Jul 30, 2018

@Eirik0
Copy link
Contributor

Eirik0 left a comment

Looks good! I suggested a couple of minor changes.

assert(res['isvalid'])
assert(res['ismine'])
assert_equal(res['type'], 'sapling')
assert(addr in self.nodes[0].z_listaddresses())

This comment has been minimized.

@Eirik0

Eirik0 Jul 30, 2018

Contributor

Something like

            types_and_addresses = [
                (default_type, self.nodes[0].z_getnewaddress()),
                ('sprout', self.nodes[0].z_getnewaddress('sprout')),
                ('sapling', self.nodes[0].z_getnewaddress('sapling'))
            ]

            all_addresses = self.nodes[0].z_listaddresses()

            for addr_type, addr in types_and_addresses:
                res = self.nodes[0].z_validateaddress(addr)
                assert(res['isvalid'])
                assert(res['ismine'])
                assert_equal(addr_type, res['type'])
                assert(addr in all_addresses)

would make it clear we are doing the same for the three scenarios.

@@ -169,7 +169,7 @@ std::pair<std::string, data> Decode(const std::string& str) {
}
if (lower && upper) return {};
size_t pos = str.rfind('1');
if (str.size() > 90 || pos == str.npos || pos == 0 || pos + 7 > str.size()) {
if (str.size() > 1023 || pos == str.npos || pos == 0 || pos + 7 > str.size()) {

This comment has been minimized.

@Eirik0

Eirik0 Jul 30, 2018

Contributor

Where does the number 1023 come from?

This comment has been minimized.

@str4d

str4d Jul 31, 2018

Contributor

From BIP 173:

Even though the chosen code performs reasonably well up to 1023 characters, other designs are preferable for lengths above 89 characters (excluding the separator).

We've decided that our addresses are generally small enough that sticking with the established Bech32 encoding for longer encodings is sufficient.

@@ -251,6 +252,19 @@ class CBasicKeyStore : public CKeyStore
virtual bool GetSaplingIncomingViewingKey(
const libzcash::SaplingPaymentAddress &addr,
libzcash::SaplingIncomingViewingKey& ivkOut) const;
void GetSaplingPaymentAddresses(std::set<libzcash::SaplingPaymentAddress> &setAddress) const

This comment has been minimized.

@Eirik0

Eirik0 Jul 30, 2018

Contributor

It would be nice if void GetPaymentAddresses(std::set<libzcash::SproutPaymentAddress> &setAddress) const were renamed to reflect that it is now Sprout specific.

This comment has been minimized.

@str4d

str4d Jul 31, 2018

Contributor

That can happen in a later PR, probably after 2.0.0 due to time pressure.

// verify there are two keys
wallet.GetSaplingPaymentAddresses(addrs);
ASSERT_EQ(2, addrs.size());
ASSERT_EQ(1, addrs.count(address));

This comment has been minimized.

@Eirik0

Eirik0 Jul 30, 2018

Contributor

What about adding ASSERT_EQ(1, addrs.count(sk.default_address()));?

"\nExamples:\n"
+ HelpExampleCli("z_getnewaddress", "")
+ HelpExampleCli("z_getnewaddress", ADDR_TYPE_SAPLING)
+ HelpExampleRpc("z_getnewaddress", "")

This comment has been minimized.

@Eirik0

Eirik0 Jul 30, 2018

Contributor

Why do we update HelpExampleCli, but not HelpExampleRpc?

This comment has been minimized.

@str4d

str4d Jul 31, 2018

Contributor

I just didn't add an example, because you can conceptually reuse the CLI examples inside the RPC ones (and pretty much everywhere else only one RPC example is given).

@@ -3100,24 +3103,40 @@ UniValue z_getnewaddress(const UniValue& params, bool fHelp)
if (!EnsureWalletIsAvailable(fHelp))
return NullUniValue;

if (fHelp || params.size() > 0)
std::string defaultType = ADDR_TYPE_SPROUT;

This comment has been minimized.

@Eirik0

Eirik0 Jul 30, 2018

Contributor

I would not be apposed to either inlining this or making it a constant.

This comment has been minimized.

@str4d

str4d Jul 31, 2018

Contributor

This was originally where we conditionally set the default based on whether Sapling had activated. We may still want to do that if we decide to make Sapling the default in 2.0.1 (after third parties have had time to test their setups). So I'll leave this as-is.

@@ -3129,7 +3148,7 @@ UniValue z_listaddresses(const UniValue& params, bool fHelp)
if (fHelp || params.size() > 1)
throw runtime_error(
"z_listaddresses ( includeWatchonly )\n"
"\nReturns the list of zaddr belonging to the wallet.\n"
"\nReturns the list of Sprout and Sapling shielded addresses belonging to the wallet.\n"

This comment has been minimized.

@Eirik0

Eirik0 Jul 30, 2018

Contributor

I like the change from "zaddr" to "shielded addresses". Neither here nor there, but we don't really need to say "Sprout and Sapling". Shouldn't that be the default unless we otherwise specify?

This comment has been minimized.

@str4d

str4d Jul 31, 2018

Contributor

I just wanted to be clearer for downstream users, given that technically previously they only ever received Sprout addresses from here, whereas now they need to check what type of address they are getting (if they are then doing something Sprout-specific, rather than using our regular APIs for which they will be interchangeable).

{
std::set<libzcash::SproutPaymentAddress> addresses;
pwalletMain->GetPaymentAddresses(addresses);
for (auto addr : addresses ) {

This comment has been minimized.

@Eirik0

Eirik0 Jul 30, 2018

Contributor

Formatting: There is an extra space after "addresses".

pwalletMain->GetSaplingPaymentAddresses(addresses);
libzcash::SaplingIncomingViewingKey ivk;
libzcash::SaplingFullViewingKey fvk;
for (auto addr : addresses ) {

This comment has been minimized.

@Eirik0

Eirik0 Jul 30, 2018

Contributor

Formatting.

str4d added some commits Jul 31, 2018

@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Aug 1, 2018

Addressed @Eirik0's comments.

@Eirik0

Eirik0 approved these changes Aug 1, 2018

@Eirik0

This comment has been minimized.

Copy link
Contributor

Eirik0 commented Aug 1, 2018

ACK.

@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Aug 1, 2018

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Aug 1, 2018

📌 Commit 4fab49e has been approved by str4d

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Aug 1, 2018

⌛️ Testing commit 4fab49e with merge d160527...

zkbot added a commit that referenced this pull request Aug 1, 2018

Auto merge of #3429 - str4d:3217-sapling-address-creation, r=str4d
Add Sapling support to z_getnewaddress and z_listaddresses

Closes #3217.
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Aug 1, 2018

💔 Test failed - pr-merge

str4d added some commits Aug 1, 2018

@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Aug 1, 2018

Pushed fixes for the two test failures - both were bugs in the tests themselves, not the code.

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Aug 1, 2018

📌 Commit 40dc060 has been approved by str4d

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Aug 1, 2018

⌛️ Testing commit 40dc060 with merge 066cc9c...

zkbot added a commit that referenced this pull request Aug 1, 2018

Auto merge of #3429 - str4d:3217-sapling-address-creation, r=str4d
Add Sapling support to z_getnewaddress and z_listaddresses

Closes #3217.
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Aug 1, 2018

☀️ Test successful - pr-merge
Approved by: str4d
Pushing 066cc9c to master...

@zkbot zkbot merged commit 40dc060 into zcash:master Aug 1, 2018

1 check passed

homu Test successful
Details

Zcashd Team automation moved this from In Review to Released (Merged in Master) Aug 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment