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

Return address and type of imported key in z_importkey #4220

Merged
merged 1 commit into from
Jan 25, 2020

Conversation

oxarbitrage
Copy link
Contributor

@oxarbitrage oxarbitrage commented Nov 11, 2019

Fixes #3748.

@oxarbitrage
Copy link
Contributor Author

I made this code last week without noticing the issue already had a PR. Please feel free to close it in favour of #3754

@oxarbitrage
Copy link
Contributor Author

oxarbitrage commented Nov 12, 2019

I ported the visitor from #3754 with some small modifications to address #3754 (comment)
Also ported the rpc_wallet_z_importkey_paymentaddress test case.

My apologies @dagurval for taking over your code. I think now this one haves the best of the 2 so #3754 can be closed if you agree.

@oxarbitrage oxarbitrage changed the title Return address of imported key in z_importkey Return address and type of imported key in z_importkey Nov 12, 2019
@zkbot
Copy link
Contributor

zkbot commented Dec 19, 2019

☔ The latest upstream changes (presumably #4270) made this pull request unmergeable. Please resolve the merge conflicts.

@r3ld3v r3ld3v added this to the Core Sprint 2020-03 milestone Jan 13, 2020
Copy link
Contributor

@str4d str4d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM.

Please address the comments below, rebase this on master (cleaning up the history as appropriate), and add a Co-authored-by: Dagur Valberg Johannsson <dagurval@pvv.ntnu.no> line to the commit messages for each commit that pulls in work from #3754.

@@ -136,11 +136,13 @@ def run_test(self):

# Verify importing a spending key will update the nullifiers and witnesses correctly
sk0 = self.nodes[0].z_exportkey(saplingAddr0)
self.nodes[2].z_importkey(sk0, "yes")
assert_equal(self.nodes[2].z_getbalance(saplingAddr0), Decimal('10'))
saplingAddr0 = self.nodes[2].z_importkey(sk0, "yes")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shadows the original saplingAddr0, functionally altering it from an address to a tuple containing an address, which is likely to confuse future people modifying this test. Use a different name for the returned tuple.

sk1 = self.nodes[1].z_exportkey(saplingAddr1)
self.nodes[2].z_importkey(sk1, "yes")
assert_equal(self.nodes[2].z_getbalance(saplingAddr1), Decimal('5'))
saplingAddr1 = self.nodes[2].z_importkey(sk1, "yes")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

Co-authored-by: Dagur Valberg Johannsson <dagurval@pvv.ntnu.no>
@oxarbitrage
Copy link
Contributor Author

Fixed comments, rebased and squashed everything in just 1 commit co authored.

@str4d str4d added S-committed Status: Planned work in a sprint A-rpc-interface Area: RPC interface A-wallet Area: Wallet labels Jan 22, 2020
Copy link
Contributor

@str4d str4d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@str4d
Copy link
Contributor

str4d commented Jan 24, 2020

@zkbot r+

@zkbot
Copy link
Contributor

zkbot commented Jan 24, 2020

📌 Commit 76a43b6 has been approved by str4d

@zkbot
Copy link
Contributor

zkbot commented Jan 25, 2020

⌛ Testing commit 76a43b6 with merge 37435aa...

zkbot added a commit that referenced this pull request Jan 25, 2020
Return address and type of imported key in z_importkey

Fixes #3748.
@zkbot
Copy link
Contributor

zkbot commented Jan 25, 2020

☀️ Test successful - pr-merge
Approved by: str4d
Pushing 37435aa to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rpc-interface Area: RPC interface A-wallet Area: Wallet S-committed Status: Planned work in a sprint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide a way to obtain the address of a key imported by z_importkey
4 participants