From 5d72a862672874312d80eb480430f5f9c0e2d622 Mon Sep 17 00:00:00 2001
From: Kris Nuttycombe <kris@nutty.land>
Date: Tue, 12 Dec 2023 14:37:29 -0700
Subject: [PATCH] Make the `Builder::build` result type consistent with the
 Sapling builder.

This avoids a panic in the case of `PaddingRule::Require(0)` or
`PaddingRule::PadTo(0)`.
---
 CHANGELOG.md     |  3 +++
 src/builder.rs   | 34 ++++++++++++++++++----------------
 tests/builder.rs |  4 ++--
 3 files changed, 23 insertions(+), 18 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 313444a81..4b7216e08 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -15,6 +15,9 @@ and this project adheres to Rust's notion of
 - `orchard::builder::Builder::add_recipient` has been renamed to `add_output`
   in order to clarify than more than one output of a given transaction may be
   sent to the same recipient.
+- `orchard::builder::Builder::build` now returns a `Result<Option<Bundle<...>>, ...>`
+  instead of a `Result<Bundle<...>, ...>` to support `PaddingRule::PadTo` and
+  to avoid a possible panic in the case of `PadTo(0)` or `Require(0)`.
 
 ## [0.6.0] - 2023-09-08
 ### Changed
diff --git a/src/builder.rs b/src/builder.rs
index c61ff34df..68046be77 100644
--- a/src/builder.rs
+++ b/src/builder.rs
@@ -32,14 +32,12 @@ const MIN_ACTIONS: usize = 2;
 #[derive(Clone, Copy, Debug, PartialEq, Eq)]
 pub enum PaddingRule {
     /// A rule that requires that at least the specified number of Orchard actions be constructed,
-    /// irrespective of whether any genuine actions are included.
+    /// irrespective of whether any genuine actions are included. If no padding is required,
+    /// then `Require(0)` may be used to indicate this.
     Require(usize),
     /// A rule that requires that at least the specified number of Orchard actions be constructed,
     /// iff any genuine actions are included.
     PadTo(usize),
-    /// A padding rule that specifies that no additional dummy Orchard actions are to be
-    /// constructed.
-    None,
 }
 
 impl PaddingRule {
@@ -62,7 +60,6 @@ impl PaddingRule {
                     core::cmp::max(num_real_actions, *n)
                 }
             }
-            PaddingRule::None => num_real_actions,
         }
     }
 }
@@ -427,10 +424,11 @@ impl Builder {
     ///
     /// The returned bundle will have no proof or signatures; these can be applied with
     /// [`Bundle::create_proof`] and [`Bundle::apply_signatures`] respectively.
+    #[allow(clippy::type_complexity)]
     pub fn build<V: TryFrom<i64>>(
         mut self,
         mut rng: impl RngCore,
-    ) -> Result<Bundle<InProgress<Unproven, Unauthorized>, V>, BuildError> {
+    ) -> Result<Option<Bundle<InProgress<Unproven, Unauthorized>, V>>, BuildError> {
         let num_real_spends = self.spends.len();
         let num_real_outputs = self.outputs.len();
         let num_actions = self
@@ -494,16 +492,18 @@ impl Builder {
         .into_bvk();
         assert_eq!(redpallas::VerificationKey::from(&bsk), bvk);
 
-        Ok(Bundle::from_parts(
-            NonEmpty::from_vec(actions).unwrap(),
-            flags,
-            result_value_balance,
-            anchor,
-            InProgress {
-                proof: Unproven { circuits },
-                sigs: Unauthorized { bsk },
-            },
-        ))
+        Ok(NonEmpty::from_vec(actions).map(|actions| {
+            Bundle::from_parts(
+                actions,
+                flags,
+                result_value_balance,
+                anchor,
+                InProgress {
+                    proof: Unproven { circuits },
+                    sigs: Unauthorized { bsk },
+                },
+            )
+        }))
     }
 }
 
@@ -861,6 +861,7 @@ pub mod testing {
             builder
                 .build(&mut self.rng)
                 .unwrap()
+                .unwrap()
                 .create_proof(&pk, &mut self.rng)
                 .unwrap()
                 .prepare(&mut self.rng, [0; 32])
@@ -973,6 +974,7 @@ mod tests {
         let bundle: Bundle<Authorized, i64> = builder
             .build(&mut rng)
             .unwrap()
+            .unwrap()
             .create_proof(&pk, &mut rng)
             .unwrap()
             .prepare(rng, [0; 32])
diff --git a/tests/builder.rs b/tests/builder.rs
index d33e2d9a9..6f438c74b 100644
--- a/tests/builder.rs
+++ b/tests/builder.rs
@@ -48,7 +48,7 @@ fn bundle_chain() {
             builder.add_output(None, recipient, NoteValue::from_raw(5000), None),
             Ok(())
         );
-        let unauthorized = builder.build(&mut rng).unwrap();
+        let unauthorized = builder.build(&mut rng).unwrap().unwrap();
         let sighash = unauthorized.commitment().into();
         let proven = unauthorized.create_proof(&pk, &mut rng).unwrap();
         proven.apply_signatures(rng, sighash, &[]).unwrap()
@@ -90,7 +90,7 @@ fn bundle_chain() {
             builder.add_output(None, recipient, NoteValue::from_raw(5000), None),
             Ok(())
         );
-        let unauthorized = builder.build(&mut rng).unwrap();
+        let unauthorized = builder.build(&mut rng).unwrap().unwrap();
         let sighash = unauthorized.commitment().into();
         let proven = unauthorized.create_proof(&pk, &mut rng).unwrap();
         proven