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_getbalance and z_gettotalbalance #3436

Merged
merged 4 commits into from Aug 25, 2018

Conversation

@str4d
Copy link
Contributor

str4d commented Aug 1, 2018

Also includes preparatory changes for various other RPCs that depend on GetFilteredNotes etc.

Closes #3214.

@str4d str4d added this to the v2.0.0 milestone Aug 1, 2018

@str4d str4d added this to Blocked in Zcashd Team Aug 1, 2018

@str4d str4d referenced this pull request Aug 1, 2018

Closed

Add Sapling support to wallet RPCs #3063

14 of 14 tasks complete
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Aug 3, 2018

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

str4d and others added some commits Aug 1, 2018

Add Sapling support to GetFilteredNotes() and GetUnspentFilteredNotes()
This could in future be refactored to be generic over PaymentAddress and
NotePlaintext in the return type, but for now let's be explicit about which
returned notes are for Sprout vs Sapling, and handle them separately.

Co-authored-by: Sean Bowe <ewillbefull@gmail.com>
Add Sapling support to z_getbalance and z_gettotalbalance
Closes #3214

Co-authored-by: Sean Bowe <ewillbefull@gmail.com>

@str4d str4d force-pushed the str4d:3214-z_getbalance branch from b06b88c to 053cb34 Aug 20, 2018

@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Aug 20, 2018

Rebased on master now that #3422 has been merged.

@str4d str4d moved this from Review Backlog to In Review in Zcashd Team Aug 20, 2018

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Aug 21, 2018

@zkbot try

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Aug 21, 2018

⌛️ Trying commit 053cb34 with merge 160555c...

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

Auto merge of #3436 - str4d:3214-z_getbalance, r=<try>
Add Sapling support to z_getbalance and z_gettotalbalance

Also includes preparatory changes for various other RPCs that depend on `GetFilteredNotes` etc.

Closes #3214.
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Aug 22, 2018

☀️ Test successful - pr-try
State: approved= try=True

@bitcartel
Copy link
Contributor

bitcartel left a comment

ACK. Minor changes to RPC help messages would be good before merging.

@@ -3344,8 +3346,8 @@ UniValue z_getbalance(const UniValue& params, bool fHelp)
throw runtime_error(
"z_getbalance \"address\" ( minconf )\n"
"\nReturns the balance of a taddr or zaddr belonging to the node’s wallet.\n"
"\nCAUTION: If address is a watch-only zaddr, the returned balance may be larger than the actual balance,"
"\nbecause spends cannot be detected with incoming viewing keys.\n"
"\nCAUTION: If address is a watch-only Sprout address, the returned balance may be larger"

This comment has been minimized.

@bitcartel

bitcartel Aug 22, 2018

Contributor

"Sprout shielded address" to avoid any confusion for users.

@@ -3408,15 +3407,15 @@ UniValue z_gettotalbalance(const UniValue& params, bool fHelp)
throw runtime_error(
"z_gettotalbalance ( minconf includeWatchonly )\n"
"\nReturn the total value of funds stored in the node’s wallet.\n"
"\nCAUTION: If the wallet contains watch-only zaddrs, the returned private balance may be larger than the actual balance,"
"\nbecause spends cannot be detected with incoming viewing keys.\n"
"\nCAUTION: If the wallet contains watch-only Sprout addresses, the returned private balance may"

This comment has been minimized.

@bitcartel

bitcartel Aug 22, 2018

Contributor

"Sprout shielded addresses"

@Eirik0 Eirik0 assigned Eirik0 and unassigned Eirik0 Aug 23, 2018

@Eirik0 Eirik0 self-requested a review Aug 23, 2018

EXPECT_EQ(1, entries.size());
entries.clear();
std::vector<CSproutNotePlaintextEntry> sproutEntries;
std::vector<SaplingNoteEntry> saplingEntries;

This comment has been minimized.

@arielgabizon

arielgabizon Aug 24, 2018

Contributor

why is it note plaintexts for sprout and notes for sapling here?

This comment has been minimized.

@str4d

str4d Aug 24, 2018

Contributor

We inevitably want to get the SaplingNote, so I decided to just return it directly.

@mdr0id

mdr0id approved these changes Aug 24, 2018

Copy link
Contributor

mdr0id left a comment

LGTM:
utACK

@daira

daira approved these changes Aug 24, 2018

Copy link
Contributor

daira left a comment

utACK modulo comments.

"\nCAUTION: If address is a watch-only zaddr, the returned balance may be larger than the actual balance,"
"\nbecause spends cannot be detected with incoming viewing keys.\n"
"\nCAUTION: If address is a watch-only Sprout shielded address, the returned balance may be larger"
"\nthan the actual balance, because spends cannot be detected with incoming viewing keys.\n"

This comment has been minimized.

@daira

daira Aug 24, 2018

Contributor

Sapling has incoming viewing keys too. The caution should be "If the wallet has only an incoming viewing key for this address, then spends cannot be detected and so the returned balance may be larger than the actual balance."

"\nCAUTION: If the wallet contains watch-only zaddrs, the returned private balance may be larger than the actual balance,"
"\nbecause spends cannot be detected with incoming viewing keys.\n"
"\nCAUTION: If the wallet contains watch-only Sprout shielded addresses, the returned private balance may"
"\nbe larger than the actual balance, because spends cannot be detected with incoming viewing keys.\n"

This comment has been minimized.

@daira

daira Aug 24, 2018

Contributor

Same comment as above.

"\nArguments:\n"
"1. minconf (numeric, optional, default=1) Only include private and transparent transactions confirmed at least this many times.\n"
"2. includeWatchonly (bool, optional, default=false) Also include balance in watchonly addresses (see 'importaddress' and 'z_importviewingkey')\n"
"\nResult:\n"
"{\n"
" \"transparent\": xxxxx, (numeric) the total balance of transparent funds\n"
" \"private\": xxxxx, (numeric) the total balance of private funds\n"
" \"private\": xxxxx, (numeric) the total balance of private funds (in both Sprout and Sapling addresses)\n"

This comment has been minimized.

@daira

daira Aug 24, 2018

Contributor

This raises the question of whether there is supposed to be a difference between "private" and "shielded" (there isn't).

This comment has been minimized.

@str4d

str4d Aug 25, 2018

Contributor

I don't quite see the context (shielded isn't mentioned in this line). Please open an issue detailing the help text rework you want (which likely needs to happen elsewhere too in order to be consistent).

This comment has been minimized.

@bitcartel

bitcartel Aug 25, 2018

Contributor

Filed #3486

@@ -4265,8 +4271,7 @@ UniValue z_mergetoaddress(const UniValue& params, bool fHelp)
maxedOutNotesFlag = true;
} else {
estimatedTxSize += increase;
// TODO: Add Sapling support
auto zaddr = boost::get<SproutPaymentAddress>(entry.address);
auto zaddr = entry.address;
SproutSpendingKey zkey;
pwalletMain->GetSproutSpendingKey(zaddr, zkey);
noteInputs.emplace_back(entry.jsop, entry.plaintext.note(zaddr), nValue, zkey);

This comment has been minimized.

@daira

daira Aug 24, 2018

Contributor

What happens for a Sapling address here?

This comment has been minimized.

@str4d

str4d Aug 24, 2018

Contributor

There is no Sapling address at this point; the loop only iterates over CSproutNotePlaintextEntry. The fact we used a boost::get<> here was AFAICT a bug (but harmless because it was a no-op).

@@ -4258,6 +4305,71 @@ void CWallet::GetUnspentFilteredNotes(
throw std::runtime_error(strprintf("Error while decrypting note for payment address %s: %s", EncodePaymentAddress(pa), exc.what()));
}
}

This comment has been minimized.

@daira

daira Aug 24, 2018

Contributor

This is (almost?) duplicated code which should be factored out.

This comment has been minimized.

@str4d

str4d Aug 25, 2018

Contributor

Yep, but that can happen at some later point.

@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Aug 25, 2018

Addressed @daira's comments.

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Aug 25, 2018

📌 Commit 573de71 has been approved by str4d

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Aug 25, 2018

⌛️ Testing commit 573de71 with merge 52bc263...

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

Auto merge of #3436 - str4d:3214-z_getbalance, r=str4d
Add Sapling support to z_getbalance and z_gettotalbalance

Also includes preparatory changes for various other RPCs that depend on `GetFilteredNotes` etc.

Closes #3214.
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Aug 25, 2018

💔 Test failed - pr-merge

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Aug 25, 2018

Transient failure in getchaintips.py on debian8.

@zkbot retry

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Aug 25, 2018

⌛️ Testing commit 573de71 with merge c53884d...

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

Auto merge of #3436 - str4d:3214-z_getbalance, r=str4d
Add Sapling support to z_getbalance and z_gettotalbalance

Also includes preparatory changes for various other RPCs that depend on `GetFilteredNotes` etc.

Closes #3214.
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Aug 25, 2018

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

@zkbot zkbot merged commit 573de71 into zcash:master Aug 25, 2018

1 check passed

homu Test successful
Details

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

@daira

This comment has been minimized.

Copy link
Contributor

daira commented Aug 27, 2018

Post-hoc ACK 573de71 .

@str4d str4d deleted the str4d:3214-z_getbalance branch Aug 27, 2018

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