-
Notifications
You must be signed in to change notification settings - Fork 252
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
zcash_client_backend: Implement note management via change splitting. #1579
zcash_client_backend: Implement note management via change splitting. #1579
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1579 +/- ##
==========================================
+ Coverage 55.93% 56.40% +0.46%
==========================================
Files 149 149
Lines 18728 19078 +350
==========================================
+ Hits 10476 10760 +284
- Misses 8252 8318 +66 ☔ View full report in Codecov by Sentry. |
6387785
to
e426b68
Compare
36a54aa
to
e6c1fa3
Compare
e6c1fa3
to
6c95fd8
Compare
force-pushed to fix the name of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flushing comments from review up to 5738990 (I'm still reviewing the last commit).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 6c95fd8.
let mut found_tx_change_memo = false; | ||
let mut found_tx_empty_memo = false; | ||
T::with_decrypted_pool_memos(&d_tx, |memo| { | ||
if Memo::try_from(memo).unwrap() == change_memo { | ||
found_tx_change_memo = true | ||
} | ||
if Memo::try_from(memo).unwrap() == Memo::Empty { | ||
found_tx_empty_memo = true | ||
} | ||
}); | ||
assert!(found_tx_change_memo); | ||
assert!(found_tx_empty_memo); | ||
|
||
// Verify that the stored sent notes match what we're expecting | ||
let sent_note_ids = st | ||
.wallet() | ||
.get_sent_note_ids(&sent_tx_id, T::SHIELDED_PROTOCOL) | ||
.unwrap(); | ||
assert_eq!(sent_note_ids.len(), 3); | ||
|
||
// The sent memo should be the empty memo for the sent output, and the | ||
// change output's memo should be as specified. | ||
let mut change_memo_count = 0; | ||
let mut found_sent_empty_memo = false; | ||
for sent_note_id in sent_note_ids { | ||
match st | ||
.wallet() | ||
.get_memo(sent_note_id) | ||
.expect("Note id is valid") | ||
.as_ref() | ||
{ | ||
Some(m) if m == &change_memo => { | ||
change_memo_count += 1; | ||
} | ||
Some(m) if m == &Memo::Empty => { | ||
found_sent_empty_memo = true; | ||
} | ||
Some(other) => panic!("Unexpected memo value: {:?}", other), | ||
None => panic!("Memo should not be stored as NULL"), | ||
} | ||
} | ||
assert_eq!(change_memo_count, 2); | ||
assert!(found_sent_empty_memo); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think nothing here checks that it is specifically the sent note (that is, the one with value 100_0000 zatoshis sent to the to
address) that has the empty memo.
eb28bfd
to
5afea2e
Compare
force-pushed to address comments from code review. Then, force-pushed to rebase on |
5afea2e
to
4059a29
Compare
force-pushed to address additional comments from my pairing with @daira. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
left some non-blocking suggestions
); | ||
|
||
// If we don't have as many change outputs as we expected, recompute the fee. | ||
let (fee_with_change, excess_fee) = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[non-blocking] This section of the code from L. 450 to L. 526 is pretty hard to read and follow (even with the generous comments). I wonder if there's a way to reduce its complexity in terms of lamdas and if-else statements so not only is more friendly to follow and maintain, but also to test.
… instead of a tuple.
…allet metadata. In the process this modifies input selection to take the change strategy as an explicit argument, rather than being wrapped as part of the input selector.
Co-authored-by: Jack Grigg <[email protected]> Co-authored-by: Daira-Emma Hopwood <[email protected]>
4059a29
to
47b1065
Compare
force-pushed to rebase on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 47b1065
} else { | ||
Ok(0) | ||
} | ||
}; | ||
|
||
// Once we calculate the balance with and without change, there are five cases: | ||
// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"change output is added" -> "change output(s) are added" on line 337.
Also on line 345, it's unclear how many change outputs will be added in that case, so the comment should be clarified (there only needs to be one for the purpose of including a change memo, but that doesn't necessarily preserve indistinguishability).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Post-hoc ACK with comments (please address them in #1590).
Co-authored-by: Daira-Emma Hopwood <[email protected]>
Co-authored-by: Daira-Emma Hopwood <[email protected]>
Best reviewed commit-by-commit, hiding whitespace changes.
Part of #1355.