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

Make Sapling the default address format when calling RPC z_getnewaddress #3680

Merged
merged 3 commits into from Nov 17, 2018

Conversation

@bitcartel
Copy link
Contributor

bitcartel commented Nov 15, 2018

Closes #3671.

@bitcartel bitcartel self-assigned this Nov 15, 2018

@bitcartel bitcartel requested review from str4d and Eirik0 Nov 15, 2018

@bitcartel bitcartel added this to the v2.0.2 milestone Nov 15, 2018

@bitcartel bitcartel requested a review from LarryRuane Nov 15, 2018

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Nov 15, 2018

Reviewers: Merge this PR last as other reviews also update tests and this PR will likely need to be rebased for those tests that have changed.

@bitcartel bitcartel added this to In Review in Zcashd Team Nov 15, 2018

@daira
Copy link
Contributor

daira left a comment

This PR seems to have a lot of unrelated test code.

Show resolved Hide resolved qa/rpc-tests/equihash_params.py Outdated
Show resolved Hide resolved qa/rpc-tests/p2p-expiringsoontx.py Outdated
Show resolved Hide resolved qa/rpc-tests/wallet_anchorfork.py.original Outdated
Show resolved Hide resolved qa/rpc-tests/wallet_large.py Outdated
Show resolved Hide resolved qa/rpc-tests/wallet_overwintertx.py.hack Outdated
Show resolved Hide resolved qa/rpc-tests/wallet_z_listreceivedbyaddress.py Outdated

@bitcartel bitcartel force-pushed the bitcartel:3671_sapling_default_z_getnewaddress branch from 33ed6b5 to fe967be Nov 15, 2018

Resolved by cleaning commit and force-pushing to PR

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Nov 15, 2018

@daira Thanks for your patience. I've cleaned this up and its now ready for review.

@@ -626,7 +626,7 @@ BOOST_AUTO_TEST_CASE(rpc_wallet_z_importexport)
BOOST_CHECK(myaddrs == listaddrs);

// Add one more address
BOOST_CHECK_NO_THROW(retValue = CallRPC("z_getnewaddress"));
BOOST_CHECK_NO_THROW(retValue = CallRPC("z_getnewaddress sprout"));
std::string newaddress = retValue.get_str();
auto address = DecodePaymentAddress(newaddress);
BOOST_CHECK(IsValidPaymentAddress(address));

This comment has been minimized.

@LarryRuane

LarryRuane Nov 15, 2018

Contributor

github doesn't let me comment on lines outside the narrow range of lines that include diffs, but I happened to notice these lines (starting at line 637) could be improved:

    // Check if too many args
    BOOST_CHECK_THROW(CallRPC("z_getnewaddress toomanyargs"), runtime_error);

This comment has been minimized.

@bitcartel

bitcartel Nov 16, 2018

Contributor

I notice that's the new idiom that is being used in some of the tests. Please file a ticket and we can improve at a future date.

This comment has been minimized.

@Eirik0

Eirik0 Nov 16, 2018

Contributor

There are lots of tests where we just check that a runtime error is thrown but not what the error was. In these cases we may or may not be testing what we are intending. I think this is the case above. It would be worth it to go and review these. Filed #3685

This comment has been minimized.

@LarryRuane

LarryRuane Nov 16, 2018

Contributor

Done, #3687. I'm not sure I understand what you mean about a new idiom; please update that issue if you think it's lacking.

@@ -626,7 +626,7 @@ BOOST_AUTO_TEST_CASE(rpc_wallet_z_importexport)
BOOST_CHECK(myaddrs == listaddrs);

// Add one more address
BOOST_CHECK_NO_THROW(retValue = CallRPC("z_getnewaddress"));
BOOST_CHECK_NO_THROW(retValue = CallRPC("z_getnewaddress sprout"));

This comment has been minimized.

@LarryRuane

LarryRuane Nov 15, 2018

Contributor

We no longer (in this source file) let the address type default (all calls toz_getnewaddress specify the type explicitly); maybe it would be better to remove the sapling argument from some of the calls just so we are sure that is indeed the default.

@@ -19,7 +19,7 @@ def run_test(self):
def addr_checks(default_type):
# Check default type, as well as explicit types
types_and_addresses = [
(default_type, self.nodes[0].z_getnewaddress()),
(default_type, self.nodes[0].z_getnewaddress('sprout')),

This comment has been minimized.

@LarryRuane

LarryRuane Nov 15, 2018

Contributor

I don't think this is the right change; I think you should leave this line unchanged but modify the five callers to this function (addr_checks()) to pass sapling instead of sprout, because the argument is exactly the default type (which is changing)..

This comment has been minimized.

@bitcartel

bitcartel Nov 16, 2018

Contributor

All the tests as they exist before this PR must explicitly ask for a sapling address, otherwise its the default sprout. Therefore, any call to z_getnewaddress() intends to receive a sprout address. Thats why I make the change here. I also ran the full test suite locally and it passed.

This comment has been minimized.

@daira

daira Nov 16, 2018

Contributor

The comment is now wrong though, since this doesn't test the default. The name of the variable is also misleading. There should be another test for the default.

This comment has been minimized.

@str4d

str4d Nov 16, 2018

Contributor

As the original author of this RPC test, I can confirm that @LarryRuane is correct: this line (unlike all other usages of z_getnewaddress with no params) is explicitly checking the default behaviour without params. See further down where I wrote Default address type is Sprout in various places; all those should now be Default address type is Sapling, with default_type changed appropriately. (For context, when I first wrote this test, the default address type returned by the API changed when Sapling activated, and this line was testing that behaviour.)

This comment has been minimized.

@bitcartel

bitcartel Nov 16, 2018

Contributor

Ah, got it now. Thanks. Fixed as @LarryRuane described. Also filed #3686 for @daira.

@str4d
Copy link
Contributor

str4d left a comment

Everything looks good; just blocking on @LarryRuane's comment on the wallet_addresses RPC test.

@Eirik0
Copy link
Contributor

Eirik0 left a comment

Requesting the same change as @str4d based on @LarryRuane's comment. Looks good otherwise.

@bitcartel bitcartel force-pushed the bitcartel:3671_sapling_default_z_getnewaddress branch from 0ac8d7f to 5a942cf Nov 16, 2018

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Nov 16, 2018

@Eirik0 @str4d Rebased and pushed to address comment.

@Eirik0

Eirik0 approved these changes Nov 16, 2018

Copy link
Contributor

Eirik0 left a comment

utACK

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Nov 16, 2018

@str4d Can you update review and once you're satisfied merge? One thing is that even when Sapling has not been activated, the default z address returned will be a Sapling one. So perhaps we should change the default based on epoch? Or maybe it doesn't matter since Sapling has activated.

@str4d

str4d approved these changes Nov 16, 2018

Copy link
Contributor

str4d left a comment

ut(ACK+cov). Comments should be fixed, but not blocking.

@@ -39,39 +39,39 @@ def addr_checks(default_type):
# Current height = 200 -> Sprout
# Default address type is Sprout

This comment has been minimized.

@str4d

str4d Nov 16, 2018

Contributor

Comment is now out-of-date.

This comment has been minimized.

@bitcartel

bitcartel Nov 16, 2018

Contributor

Added this to #3686

This comment has been minimized.

@bitcartel

bitcartel Nov 16, 2018

Contributor

Scratch that, I will fix now.

@@ -39,39 +39,39 @@ def addr_checks(default_type):
# Current height = 200 -> Sprout
# Default address type is Sprout
print "Testing height 200 (Sprout)"
addr_checks('sprout')
addr_checks('sapling')

self.nodes[0].generate(1)
self.sync_all()

# Current height = 201 -> Sprout
# Default address type is Sprout

This comment has been minimized.

@str4d

str4d Nov 16, 2018

Contributor

Comment is now out-of-date.


self.nodes[0].generate(1)
self.sync_all()

# Current height = 201 -> Sprout
# Default address type is Sprout
print "Testing height 201 (Sprout)"
addr_checks('sprout')
addr_checks('sapling')

self.nodes[0].generate(1)
self.sync_all()

# Current height = 202 -> Overwinter
# Default address type is Sprout

This comment has been minimized.

@str4d

str4d Nov 16, 2018

Contributor

Comment is now out-of-date.


self.nodes[0].generate(1)
self.sync_all()

# Current height = 202 -> Overwinter
# Default address type is Sprout
print "Testing height 202 (Overwinter)"
addr_checks('sprout')
addr_checks('sapling')

self.nodes[0].generate(1)
self.sync_all()

# Current height = 203 -> Overwinter
# Default address type is Sprout

This comment has been minimized.

@str4d

str4d Nov 16, 2018

Contributor

Comment is now out-of-date.


self.nodes[0].generate(1)
self.sync_all()

# Current height = 203 -> Overwinter
# Default address type is Sprout
print "Testing height 203 (Overwinter)"
addr_checks('sprout')
addr_checks('sapling')

self.nodes[0].generate(1)
self.sync_all()

# Current height = 204 -> Sapling
# Default address type is Sprout

This comment has been minimized.

@str4d

str4d Nov 16, 2018

Contributor

Comment is now out-of-date.

@bitcartel bitcartel force-pushed the bitcartel:3671_sapling_default_z_getnewaddress branch from 5a942cf to 12a1267 Nov 16, 2018

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Nov 16, 2018

@str4d review comments addressed.

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Nov 16, 2018

📌 Commit 12a1267 has been approved by bitcartel

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Nov 16, 2018

⌛️ Testing commit 12a1267 with merge 8394c30...

zkbot added a commit that referenced this pull request Nov 16, 2018

Auto merge of #3680 - bitcartel:3671_sapling_default_z_getnewaddress,…
… r=bitcartel

Make Sapling the default address format when calling RPC z_getnewaddress

Closes #3671.
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Nov 17, 2018

☀️ Test successful - pr-merge
Approved by: bitcartel
Pushing 8394c30 to master...

@zkbot zkbot merged commit 12a1267 into zcash:master Nov 17, 2018

1 check passed

homu Test successful
Details

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

@zkbot zkbot referenced this pull request Nov 17, 2018

Open

3553 listnotes rpc test #3569

@daira

This comment has been minimized.

Copy link
Contributor

daira commented Nov 19, 2018

@bitcartel wrote:

@str4d Can you update review and once you're satisfied merge? One thing is that even when Sapling has not been activated, the default z address returned will be a Sapling one. So perhaps we should change the default based on epoch? Or maybe it doesn't matter since Sapling has activated.

It does matter for forks that haven't yet activated Sapling. We should change the default based on the epoch at max(APPROX_RELEASE_HEIGHT, currentHeight).

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