Skip to content
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

Fix z_mergetoaddress sending from ANY_SPROUT/ANY_SAPLING when the wallet contains both note types #3910

Merged
merged 2 commits into from Jun 10, 2019

Conversation

@Eirik0
Copy link
Contributor

commented Mar 22, 2019

Closes #3909

@Eirik0 Eirik0 added this to Needs Prioritization in Arborist Team via automation Mar 22, 2019

@Eirik0 Eirik0 self-assigned this Mar 22, 2019

@Eirik0

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

The issue was that when using ANY_SPROUT/ANY_SAPLING we were just checking if pwalletMain->GetFilteredNotes(sproutEntries, saplingEntries, zaddrs) returned both sprout and sapling notes and throwing an error if it did (not taking in to account which parameter was passed in).

@ioptio

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2019

We should specify in the payment API the combination of address types that can work with this call. https://zcash.readthedocs.io/en/latest/rtd_pages/payment_api.html#z-mergetoaddress

@ioptio ioptio added this to Blocked/Tracking in Documentation Mar 28, 2019

@mms710 mms710 moved this from Needs Prioritization to Blocked in Arborist Team Apr 1, 2019

@mms710 mms710 moved this from Blocked to Needs Prioritization in Arborist Team Apr 1, 2019

@mms710 mms710 moved this from Needs Prioritization to Bugs/Security Issues/TechDebt Backlog in Arborist Team Apr 11, 2019

@mms710 mms710 moved this from Bugs/Security Issues/TechDebt Backlog to PRs That Need Review + Their Associated Issues in Arborist Team Apr 11, 2019

@Eirik0 Eirik0 requested review from daira, str4d and LarryRuane Jun 6, 2019

@Eirik0 Eirik0 moved this from PRs That Need Review + Their Associated Issues to In Review in Arborist Team Jun 6, 2019

@Eirik0 Eirik0 moved this from In Review to In Progress in Arborist Team Jun 6, 2019

@Eirik0

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

@ioptio I updated the documentation in the RPC help. If that looks good, I will submit something similar to the documentation repo.

@str4d

str4d approved these changes Jun 7, 2019

Copy link
Contributor

left a comment

ut(ACK+cov). This is a sufficient fix until we migrate over to using TransactionBuilder for everything.

@daira
Copy link
Contributor

left a comment

Missing test case. Otherwise utACK.

qa/rpc-tests/mergetoaddress_helper.py Show resolved Hide resolved
@LarryRuane
Copy link
Contributor

left a comment

Nice PR! ACK 335376f, verified that the test succeeds with the changes, and fails without them.

@Eirik0 Eirik0 requested a review from daira Jun 10, 2019

@Eirik0 Eirik0 force-pushed the Eirik0:3909-fix-mergetoaddress branch from ebbb905 to b087bd3 Jun 10, 2019

@Eirik0

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

I copied and pasted a comment and forgot to update Sprout to Sapling. Force pushed to fix this.

@Eirik0 Eirik0 moved this from In Progress to In Review in Arborist Team Jun 10, 2019

@@ -45,7 +46,11 @@ def run_test(self):

assert_equal(Decimal(self.nodes[1].z_gettotalbalance()["transparent"]), Decimal('0'))

# Send to t_addr from sprout
# Make sure we cannot use "ANY_SPROUT" and "ANY_SAPLING" even if we only have Sprout Notes

This comment has been minimized.

Copy link
@daira

daira Jun 10, 2019

Contributor

At this point in the test we have both Sprout and Sapling notes (10 ZEC each), I think.

This comment has been minimized.

Copy link
@Eirik0

Eirik0 Jun 10, 2019

Author Contributor

You are correct. Fixing now.

@Eirik0

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

I will squash these commits together after rereview.

@Eirik0 Eirik0 requested a review from daira Jun 10, 2019

@daira

daira approved these changes Jun 10, 2019

Copy link
Contributor

left a comment

ut(ACK+cov)

@str4d

str4d approved these changes Jun 10, 2019

Copy link
Contributor

left a comment

ut(ACK+cov)

@Eirik0 Eirik0 force-pushed the Eirik0:3909-fix-mergetoaddress branch from 9264324 to cf30355 Jun 10, 2019

@Eirik0

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Jun 10, 2019

📌 Commit cf30355 has been approved by Eirik0

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Jun 10, 2019

⌛️ Testing commit cf30355 with merge 5f40d5e...

zkbot added a commit that referenced this pull request Jun 10, 2019

Auto merge of #3910 - Eirik0:3909-fix-mergetoaddress, r=Eirik0
Fix z_mergetoaddress sending from ANY_SPROUT/ANY_SAPLING when the wallet contains both note types

Closes #3909

@str4d str4d added this to the v2.0.6 milestone Jun 10, 2019

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Jun 10, 2019

☀️ Test successful - pr-merge
Approved by: Eirik0
Pushing 5f40d5e to master...

@zkbot zkbot merged commit cf30355 into zcash:master Jun 10, 2019

1 check passed

homu Test successful
Details

Documentation automation moved this from Blocked/Tracking to Complete Jun 10, 2019

Arborist Team automation moved this from In Review to Released (Merged in Master) Jun 10, 2019

@Eirik0 Eirik0 modified the milestones: v2.0.6, Sapling Specification Jun 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.