Update the transaction builder to make change outputs explicit#658
Conversation
nuttycom
force-pushed
the
wallet/builder_explicit_change
branch
2 times, most recently
from
82ff5fb to
ea95514
Compare
nuttycom
force-pushed
the
wallet/builder_explicit_change
branch
from
ea95514 to
1c12d71
Compare
Codecov ReportBase: 73.66% // Head: 72.17% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #658 +/- ##
==========================================
- Coverage 73.66% 72.17% -1.50%
==========================================
Files 104 110 +6
Lines 10157 11097 +940
==========================================
+ Hits 7482 8009 +527
- Misses 2675 3088 +413
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/builder_explicit_change
branch
from
1c12d71 to
e849928
Compare
nuttycom
force-pushed
the
wallet/builder_explicit_change
branch
from
e849928 to
f90c64d
Compare
nuttycom
force-pushed
the
wallet/builder_explicit_change
branch
3 times, most recently
from
de7dde5 to
f807eae
Compare
nuttycom
changed the title
daira
left a comment
daira
left a comment
There was a problem hiding this comment.
utACK modulo the comments from my previous review. In particular there are still some bare uses of DEFAULT_FEE that assume fees are fixed without going through FixedFeeStrategy.
str4d
left a comment
str4d
left a comment
There was a problem hiding this comment.
Partial review of 6280ddebc556ef06064e603c2f04b4e43d24bd73.
str4d
left a comment
str4d
left a comment
There was a problem hiding this comment.
Reviewed 6280ddebc556ef06064e603c2f04b4e43d24bd73.
nuttycom
force-pushed
the
wallet/builder_explicit_change
branch
2 times, most recently
from
3a88999 to
a55743d
Compare
nuttycom
force-pushed
the
wallet/builder_explicit_change
branch
from
a55743d to
324e1d5
Compare
nuttycom
force-pushed
the
wallet/builder_explicit_change
branch
3 times, most recently
from
b7fbe73 to
47b43dd
Compare
This adds a fee calculation strategy abstraction that can be used to dynamically compute fees so that the total fees required may be taken taken into account during note selection, and also removes automatic change creation from the transaction builder. Change outputs must now be directly created by the caller by the caller. This is a necessary prerequisite for permitting fees to adjust based upon the contents of the transaction being constructed. The initial implementation of the fee strategy simply uses the current default fee.
nuttycom
force-pushed
the
wallet/builder_explicit_change
branch
from
47b43dd to
9496fc6
Compare
|
force-pushed to address review comments. |
| /// The amount of change and fees required to make a transaction's inputs and | ||
| /// outputs balance under a specific fee rule, as computed by a particular | ||
| /// [`ChangeStrategy`] that is aware of that rule. | ||
| pub struct TransactionBalance { | ||
| proposed_change: Vec<ChangeValue>, | ||
| fee_required: Amount, | ||
| } |
There was a problem hiding this comment.
I like this approach; it's going to enable note splitting easily. I'm now wondering about note merging; ChangeStrategy assumes that all of the provided inputs will be part of the transaction (which makes sense given that it is focused on change strategy), so does this mean to have a variable number of inputs beyond what is necessary, we will need a third layer around ChangeStrategy and FeeRule? Or would it make more sense to do both note merging and note splitting in the same place? I guess the outer layer could equivalently implement that itself...
There was a problem hiding this comment.
Your inference is correct: we do need a third layer! See #689
| .map_err(|e| match e { | ||
| ChangeError::InsufficientFunds { | ||
| available, | ||
| required, | ||
| } => Error::InsufficientBalance(available, required), | ||
| ChangeError::StrategyError(e) => Error::BalanceError(e), | ||
| })?; |
There was a problem hiding this comment.
We do this match in two places, so we should add an impl From<ChangeError> for Error.
There was a problem hiding this comment.
Error handling changes substantially in #689 so let's wait to alter this.
| .add_sapling_output( | ||
| Some(dfvk.to_ovk(Scope::Internal)), | ||
| dfvk.change_address().1, | ||
| *amount, |
There was a problem hiding this comment.
Why does this need to be dereferenced?
There was a problem hiding this comment.
proposed_change returns a slice, and so it's borrowed here.
| match change_value { | ||
| ChangeValue::Sapling(amount) => { | ||
| builder | ||
| .add_sapling_output(Some(ovk), shielding_address.clone(), *amount, memo.clone()) |
There was a problem hiding this comment.
See above; proposed_change returns a slice.
nuttycom
force-pushed
the
wallet/builder_explicit_change
branch
2 times, most recently
from
ae95f77 to
11889f7
Compare
nuttycom
force-pushed
the
wallet/builder_explicit_change
branch
from
11889f7 to
c92d81b
Compare
|
updated to address requested changes. |
3 participants
This introduces a FeeStrategy abstraction (and
FutureFeeStrategyforzfuture) that can be used to compute fees and required change during the process of transaction build. This also modifiesTransactionBuilderto require the caller to construct a change output directly, instead of doing so implicitly.