Add ZIP 317 fee calculation strategy.#694
Conversation
Codecov ReportBase: 72.58% // Head: 71.38% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #694 +/- ##
==========================================
- Coverage 72.58% 71.38% -1.21%
==========================================
Files 107 115 +8
Lines 10932 11531 +599
==========================================
+ Hits 7935 8231 +296
- Misses 2997 3300 +303
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
nuttycom
force-pushed
the
wallet/zip_317
branch
7 times, most recently
from
0bffbc1 to
041e457
Compare
nuttycom
force-pushed
the
wallet/zip_317
branch
5 times, most recently
from
b6a5039 to
bac6024
Compare
str4d
left a comment
str4d
left a comment
There was a problem hiding this comment.
Partial review of bac6024f9e8b7fc93e2edd42a0b593829e53a299 (finished everything except zcash_client_backend::fees::zip317).
There was a problem hiding this comment.
Bug: this assumes the kind of each transparent input and output, such that p2pkh_standard_input_size and p2pkh_standard_output_size cancel out and cause the transparent contribution to only use the number of inputs and outputs. Instead it needs to use the as-serialized-in-transaction size of transparent_inputs and transparent_outputs (which can be calculated from the data accessible via transparent::{inputView, OutputView}).
There was a problem hiding this comment.
I mean, this isn't a bug, this is a known not-fully-implemented. What I didn't implement was the DER encoding, because won't that vary based upon the value of the signature, which we can't get here?
There was a problem hiding this comment.
Resolved by making passing anything but a p2pkh input an error.
This adds a set of abstractions that allow wallets to provide independent strategies for fee estimation and note selection, and implementations of these strategies that perform these operations in the same fashion as the existing `spend` and `shield_transparent_funds` functions. This required a somewhat hefty rework of the error handling in zcash_client_backend. It fixes an issue with the error types whereby callees needed to have a bit too much information about the error types produced by their callers. Reflect the updated note selection and error handling in zcash_client_sqlite.
The change selection algorithm has the most useful information for determining whether or not a note is dust, so this adds a new error case to `ChangeError` that allows the change selection to report the presence of input notes without economic value back to its caller.
nuttycom
force-pushed
the
wallet/zip_317
branch
3 times, most recently
from
8136e6a to
df91b9b
Compare
nuttycom
force-pushed
the
wallet/zip_317
branch
from
df91b9b to
73ab884
Compare
| ), | ||
| Err(Error::InsufficientFunds { available, required }) | ||
| if available == Amount::from_u64(51000).unwrap() | ||
| && required == Amount::from_u64(60000).unwrap() |
There was a problem hiding this comment.
A note here: we do not yet do anything to indicate that notes have been rejected as dust when performing wallet balance calculation. We might want to do that.
| // if we have available grace inputs, allocate them first to transparent dust | ||
| // and then to sapling dust |
There was a problem hiding this comment.
To clarify, the assumption here is that if any transparent inputs have been provided to this ChangeStrategy, the caller is okay with any of those inputs being spent, so we don't need to consider the privacy implications here. It might be useful to call these out in a comment.
3 participants
This builds atop #689