Apply suggestions from code review & zcash#1579

Co-authored-by: Daira-Emma Hopwood <[email protected]>
nuttycom · Oct 25, 2024 · ae58d3e · ae58d3e
1 parent a12b75e
commit ae58d3e
Show file tree

Hide file tree

Showing 6 changed files with 20 additions and 15 deletions.
diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md
@@ -64,6 +64,8 @@ and this library adheres to Rust's notion of
     and `WalletMeta`, and its `FeeRule` associated type now has an additional
     `Clone` bound. In addition, it defines a new `fetch_wallet_meta` method, and
     the arguments to `compute_balance` have changed.
+  - `zip317::SingleOutputChangeStrategy` has been made polymorphic in the fee
+    rule type, and takes an additional type parameter as a consequence.
   - The following methods now take an additional `DustOutputPolicy` argument,
     and carry an additional type parameter:
     - `fixed::SingleOutputChangeStrategy::new`
@@ -72,10 +74,8 @@ and this library adheres to Rust's notion of
 - `zcash_client_backend::proto::ProposalDecodingError` has modified variants.
   `ProposalDecodingError::FeeRuleNotSpecified` has been removed, and
   `ProposalDecodingError::FeeRuleNotSupported` has been added to replace it.
-
-### Deprecated
-- `zcash_client_backend::data_api::fees::fixed`; also, this module is now
-  available only via the use of the `non-standard-fees` feature flag.
+- `zcash_client_backend::data_api::fees::fixed` is now available only via the
+  use of the `non-standard-fees` feature flag.
 
 ### Removed
 - `zcash_client_backend::data_api`:

diff --git a/zcash_client_backend/src/fees/common.rs b/zcash_client_backend/src/fees/common.rs
@@ -500,7 +500,7 @@ where
                 .dust_threshold()
                 .unwrap_or(cfg.default_dust_threshold);
 
-            if per_output_change.quotient() < &change_dust_threshold {
+            if proposed_change < change_dust_threshold {
                 match cfg.dust_output_policy.action() {
                     DustAction::Reject => {
                         // Always allow zero-valued change even for the `Reject` policy:
@@ -509,11 +509,11 @@ where
                         // * this case occurs in practice when sending all funds from an account;
                         // * zero-valued notes do not require witness tracking;
                         // * the effect on trial decryption overhead is small.
-                        if per_output_change.quotient().is_zero() {
+                        if proposed_change.is_zero() && excess_fee.is_zero() {
                             simple_case()
                         } else {
-                            let shortfall = (change_dust_threshold - *per_output_change.quotient())
-                                .ok_or_else(underflow)?;
+                            let shortfall =
+                                (change_dust_threshold - proposed_change).ok_or_else(underflow)?;
 
                             return Err(ChangeError::InsufficientFunds {
                                 available: total_in,

diff --git a/zcash_client_sqlite/src/wallet/init/migrations/fix_bad_change_flagging.rs b/zcash_client_sqlite/src/wallet/init/migrations/fix_bad_change_flagging.rs
@@ -86,15 +86,12 @@ mod tests {
                 wallet::input_selection::GreedyInputSelector,
                 Account as _, WalletRead as _, WalletWrite as _,
             },
-            fees::{standard, DustOutputPolicy},
+            fees::{standard, DustOutputPolicy, StandardFeeRule},
             wallet::WalletTransparentOutput,
         },
         zcash_primitives::{
             block::BlockHash,
-            transaction::{
-                components::{OutPoint, TxOut},
-                fees::StandardFeeRule,
-            },
+            transaction::components::{OutPoint, TxOut},
         },
         zcash_protocol::value::Zatoshis,
     };

diff --git a/zcash_client_sqlite/src/wallet/orchard.rs b/zcash_client_sqlite/src/wallet/orchard.rs
@@ -401,7 +401,7 @@ pub(crate) mod tests {
 
     #[test]
     fn send_with_multiple_change_outputs() {
-        testing::pool::send_with_multiple_change_outputs::<SaplingPoolTester>()
+        testing::pool::send_with_multiple_change_outputs::<OrchardPoolTester>()
     }
 
     #[test]

diff --git a/zcash_primitives/CHANGELOG.md b/zcash_primitives/CHANGELOG.md
@@ -31,7 +31,7 @@ and this library adheres to Rust's notion of
 ### Removed
 - `zcash_primitives::transaction::fees`:
   - `StandardFeeRule` itself has been removed; it was not used in this crate.
-    Is use in `zcash_client_backend` has been replaced with
+    Its use in `zcash_client_backend` has been replaced with
     `zcash_client_backend::fees::StandardFeeRule`.
   - `fixed::FeeRule::standard`. This constructor was misleadingly named: using a
     fixed fee does not conform to any current Zcash standard. To calculate the

diff --git a/zcash_primitives/src/transaction/fees/zip317.rs b/zcash_primitives/src/transaction/fees/zip317.rs
@@ -76,6 +76,14 @@ impl FeeRule {
 
     /// Construct a new FeeRule instance with the specified parameter values.
     ///
+    /// Using this fee rule with
+    /// ```compile_fail
+    /// marginal_fee < 5000 || grace_actions < 2 \
+    ///     || p2pkh_standard_input_size > P2PKH_STANDARD_INPUT_SIZE \
+    ///     || p2pkh_standard_output_size > P2PKH_STANDARD_OUTPUT_SIZE
+    /// ```
+    /// violates ZIP 317, and might cause transactions built with it to fail.
+    ///
     /// Returns `None` if either `p2pkh_standard_input_size` or `p2pkh_standard_output_size` are
     /// zero.
     #[cfg(feature = "non-standard-fees")]