Address the latest PR #104 review comments

benches/circuit.rs

@@ -11,7 +11,7 @@ use orchard::{
     circuit::{ProvingKey, VerifyingKey},
     keys::{FullViewingKey, Scope, SpendingKey},
     note::AssetBase,
-    orchard_flavors::{OrchardFlavor, OrchardVanilla, OrchardZSA},
+    orchard_flavor::{OrchardFlavor, OrchardVanilla, OrchardZSA},
     value::NoteValue,
     Anchor, Bundle,
 };

benches/note_decryption.rs

@@ -5,7 +5,7 @@ use orchard::{
     keys::{FullViewingKey, PreparedIncomingViewingKey, Scope, SpendingKey},
     note::AssetBase,
     note_encryption::{CompactAction, OrchardDomain},
-    orchard_flavors::{OrchardFlavor, OrchardVanilla, OrchardZSA},
+    orchard_flavor::{OrchardFlavor, OrchardVanilla, OrchardZSA},
     value::NoteValue,
     Anchor, Bundle,
 };

src/action.rs

@@ -133,7 +133,7 @@ pub(crate) mod testing {
     use crate::{
         note::{
             asset_base::testing::arb_asset_base, commitment::ExtractedNoteCommitment,
-            nullifier::testing::arb_nullifier, testing::arb_note, TransmittedNoteCiphertext,
+            nullifier::testing::arb_nullifier, testing::arb_note, Note, TransmittedNoteCiphertext,
         },
         note_encryption::{OrchardDomain, OrchardDomainCommon},
         primitives::redpallas::{
@@ -145,20 +145,22 @@ pub(crate) mod testing {
 
     use super::Action;
 
-    /// `ActionArb` adapts `arb_...` functions for both Vanilla and ZSA Orchard protocol variations
-    /// in property-based testing, addressing proptest crate limitations.
     #[derive(Debug)]
     pub struct ActionArb<D: OrchardDomainCommon> {
         phantom: std::marker::PhantomData<D>,
     }
 
     impl<D: OrchardDomainCommon> ActionArb<D> {
         fn encrypt_note<R: RngCore>(
-            encryptor: NoteEncryption<OrchardDomain<D>>,
+            note: Note,
+            memo: Vec<u8>,
             cmx: &ExtractedNoteCommitment,
             cv_net: &ValueCommitment,
             rng: &mut R,
         ) -> TransmittedNoteCiphertext<D> {
+            let encryptor =
+                NoteEncryption::<OrchardDomain<D>>::new(None, note, memo.try_into().unwrap());
+
             TransmittedNoteCiphertext {
                 epk_bytes: encryptor.epk().to_bytes().0,
                 enc_ciphertext: encryptor.encrypt_note_plaintext(),
@@ -184,8 +186,7 @@ pub(crate) mod testing {
                 );
 
                 let mut rng = StdRng::from_seed(rng_seed);
-                let encryptor = NoteEncryption::<OrchardDomain<D>>::new(None, note, memo.try_into().unwrap());
-                let encrypted_note = Self::encrypt_note(encryptor, &cmx, &cv_net, &mut rng);
+                let encrypted_note = Self::encrypt_note(note, memo, &cmx, &cv_net, &mut rng);
 
                 Action {
                     nf,
@@ -218,8 +219,7 @@ pub(crate) mod testing {
 
                 let mut rng = StdRng::from_seed(rng_seed);
 
-                let encryptor = NoteEncryption::<OrchardDomain<D>>::new(None, note, memo.try_into().unwrap());
-                let encrypted_note = Self::encrypt_note(encryptor, &cmx, &cv_net, &mut rng);
+                let encrypted_note = Self::encrypt_note(note, memo, &cmx, &cv_net, &mut rng);
 
                 Action {
                     nf,

src/builder.rs

@@ -24,7 +24,7 @@ use crate::{
     },
     note::{AssetBase, Note, Rho, TransmittedNoteCiphertext},
     note_encryption::{OrchardDomain, OrchardDomainCommon},
-    orchard_flavors::OrchardFlavor,
+    orchard_flavor::OrchardFlavor,
     primitives::redpallas::{self, Binding, SpendAuth},
     tree::{Anchor, MerklePath},
     value::{self, NoteValue, OverflowError, ValueCommitTrapdoor, ValueCommitment, ValueSum},
@@ -113,8 +113,6 @@ impl BundleType {
     pub fn flags(&self) -> Flags {
         match self {
             BundleType::Transactional { flags, .. } => *flags,
-            // FIXME: is it correct to use SPENDS_DISABLED_WITHOUT_ZSA (i.e does coinbase always have
-            // ZSA disabled)?
             BundleType::Coinbase => Flags::SPENDS_DISABLED_WITHOUT_ZSA,
         }
     }
@@ -637,7 +635,6 @@ 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>, FL: OrchardFlavor>(
         self,
         rng: impl RngCore,
@@ -852,19 +849,14 @@ pub fn bundle<V: TryFrom<i64>, FL: OrchardFlavor>(
                 asset,
                 NoteValue::from_raw(
                     u64::try_from(i128::from(value))
-                        .map_err(|_| BuildError::ValueSum(value::OverflowError))?,
+                        .map_err(|_| BuildError::ValueSum(OverflowError))?,
                 ),
             ))
         })
         .collect::<Result<Vec<(AssetBase, NoteValue)>, BuildError>>()?;
 
     // Verify that bsk and bvk are consistent.
-    let bvk = derive_bvk(
-        &actions,
-        native_value_balance,
-        burn.iter().cloned(),
-        //burn.iter().flat_map(|(asset, value)| -> Result<_, BuildError> { Ok((*asset, (*value).into()?)) }),
-    );
+    let bvk = derive_bvk(&actions, native_value_balance, burn.iter().cloned());
     assert_eq!(redpallas::VerificationKey::from(&bsk), bvk);
 
     Ok(NonEmpty::from_vec(actions).map(|actions| {
@@ -935,15 +927,13 @@ impl<S: InProgressSignatures, C: OrchardCircuit> InProgress<Unproven<C>, S> {
     }
 }
 
-impl<S: InProgressSignatures, V, D: OrchardDomainCommon + OrchardCircuit>
-    Bundle<InProgress<Unproven<D>, S>, V, D>
-{
+impl<S: InProgressSignatures, V, FL: OrchardFlavor> Bundle<InProgress<Unproven<FL>, S>, V, FL> {
     /// Creates the proof for this bundle.
     pub fn create_proof(
         self,
         pk: &ProvingKey,
         mut rng: impl RngCore,
-    ) -> Result<Bundle<InProgress<Proof, S>, V, D>, BuildError> {
+    ) -> Result<Bundle<InProgress<Proof, S>, V, FL>, BuildError> {
         let instances: Vec<_> = self
             .actions()
             .iter()
@@ -1208,7 +1198,7 @@ pub mod testing {
         keys::{testing::arb_spending_key, FullViewingKey, SpendAuthorizingKey, SpendingKey},
         note::testing::arb_note,
         note_encryption::OrchardDomainCommon,
-        orchard_flavors::OrchardFlavor,
+        orchard_flavor::OrchardFlavor,
         tree::{Anchor, MerkleHashOrchard, MerklePath},
         value::{testing::arb_positive_note_value, NoteValue, MAX_NOTE_VALUE},
         Address, Note,
@@ -1355,7 +1345,7 @@ mod tests {
         constants::MERKLE_DEPTH_ORCHARD,
         keys::{FullViewingKey, Scope, SpendingKey},
         note::AssetBase,
-        orchard_flavors::{OrchardFlavor, OrchardVanilla, OrchardZSA},
+        orchard_flavor::{OrchardFlavor, OrchardVanilla, OrchardZSA},
         tree::EMPTY_ROOTS,
         value::NoteValue,
     };

src/bundle.rs

@@ -21,6 +21,7 @@ use crate::{
     keys::{IncomingViewingKey, OutgoingViewingKey, PreparedIncomingViewingKey},
     note::{AssetBase, Note},
     note_encryption::{OrchardDomain, OrchardDomainCommon},
+    orchard_flavor::OrchardFlavor,
     primitives::redpallas::{self, Binding, SpendAuth},
     tree::Anchor,
     value::{NoteValue, ValueCommitTrapdoor, ValueCommitment, ValueSum},
@@ -203,7 +204,6 @@ pub struct Bundle<A: Authorization, V, D: OrchardDomainCommon> {
     /// This is the sum of Orchard spends minus the sum of Orchard outputs.
     value_balance: V,
     /// Assets intended for burning
-    // FIXME: use BurnType like it's in Zebra? Put it as another param of Domain trait
     burn: Vec<(AssetBase, NoteValue)>,
     /// The root of the Orchard commitment tree that this bundle commits to.
     anchor: Anchor,
@@ -484,13 +484,8 @@ impl<A: Authorization, V, D: OrchardDomainCommon> Bundle<A, V, D> {
     }
 }
 
-pub(crate) fn derive_bvk<
-    'a,
-    A: 'a,
-    V: Clone + Into<i64>,
-    D: 'a + OrchardDomainCommon + OrchardHash,
->(
-    actions: impl IntoIterator<Item = &'a Action<A, D>>,
+pub(crate) fn derive_bvk<'a, A: 'a, V: Clone + Into<i64>, FL: 'a + OrchardFlavor>(
+    actions: impl IntoIterator<Item = &'a Action<A, FL>>,
     value_balance: V,
     burn: impl Iterator<Item = (AssetBase, NoteValue)>,
 ) -> redpallas::VerificationKey<Binding> {
@@ -512,7 +507,7 @@ pub(crate) fn derive_bvk<
     .into_bvk()
 }
 
-impl<A: Authorization, V: Copy + Into<i64>, D: OrchardDomainCommon + OrchardHash> Bundle<A, V, D> {
+impl<A: Authorization, V: Copy + Into<i64>, FL: OrchardFlavor> Bundle<A, V, FL> {
     /// Computes a commitment to the effects of this bundle, suitable for inclusion within
     /// a transaction ID.
     pub fn commitment(&self) -> BundleCommitment {

src/bundle/batch.rs

@@ -6,9 +6,8 @@ use tracing::debug;
 use super::{Authorized, Bundle};
 
 use crate::{
-    bundle::OrchardHash,
     circuit::VerifyingKey,
-    note_encryption::OrchardDomainCommon,
+    orchard_flavor::OrchardFlavor,
     primitives::redpallas::{self, Binding, SpendAuth},
 };
 
@@ -38,9 +37,9 @@ impl BatchValidator {
     }
 
     /// Adds the proof and RedPallas signatures from the given bundle to the validator.
-    pub fn add_bundle<V: Copy + Into<i64>, D: OrchardDomainCommon + OrchardHash>(
+    pub fn add_bundle<V: Copy + Into<i64>, FL: OrchardFlavor>(
         &mut self,
-        bundle: &Bundle<Authorized, V, D>,
+        bundle: &Bundle<Authorized, V, FL>,
         sighash: [u8; 32],
     ) {
         for action in bundle.actions().iter() {

src/bundle/commitments.rs

@@ -9,7 +9,7 @@ use crate::{
     issuance::{IssueAuth, IssueBundle, Signed},
     note::AssetBase,
     note_encryption::OrchardDomainCommon,
-    orchard_flavors::{OrchardVanilla, OrchardZSA},
+    orchard_flavor::{OrchardVanilla, OrchardZSA},
     value::NoteValue,
 };
 
@@ -100,7 +100,6 @@ pub(crate) fn hash_bundle_txid_data<
     h.update(mh.finalize().as_bytes());
     h.update(nh.finalize().as_bytes());
 
-    // Delegate complete handling of the burn data to the OrchardHash implementation
     D::update_hash_with_burn(&mut h, &bundle.burn);
 
     h.update(&[bundle.flags().to_byte()]);

src/circuit/circuit_vanilla.rs

@@ -32,7 +32,7 @@ use crate::{
     constants::{
         OrchardCommitDomains, OrchardFixedBases, OrchardFixedBasesFull, OrchardHashDomains,
     },
-    orchard_flavors::OrchardVanilla,
+    orchard_flavor::OrchardVanilla,
 };
 
 use super::{
@@ -658,7 +658,7 @@ mod tests {
         circuit::{Circuit, Instance, Proof, ProvingKey, VerifyingKey, K},
         keys::SpendValidatingKey,
         note::{AssetBase, Note, Rho},
-        orchard_flavors::OrchardVanilla,
+        orchard_flavor::OrchardVanilla,
         tree::MerklePath,
         value::{ValueCommitTrapdoor, ValueCommitment},
     };

src/circuit/circuit_zsa.rs

@@ -38,7 +38,7 @@ use crate::{
     constants::OrchardFixedBasesFull,
     constants::{OrchardCommitDomains, OrchardFixedBases, OrchardHashDomains},
     note::AssetBase,
-    orchard_flavors::OrchardZSA,
+    orchard_flavor::OrchardZSA,
 };
 
 use super::{
@@ -897,7 +897,7 @@ mod tests {
         circuit::{Circuit, Instance, Proof, ProvingKey, VerifyingKey, K},
         keys::{FullViewingKey, Scope, SpendValidatingKey, SpendingKey},
         note::{commitment::NoteCommitTrapdoor, AssetBase, Note, NoteCommitment, Nullifier, Rho},
-        orchard_flavors::OrchardZSA,
+        orchard_flavor::OrchardZSA,
         primitives::redpallas::VerificationKey,
         tree::MerklePath,
         value::{NoteValue, ValueCommitTrapdoor, ValueCommitment},

src/lib.rs

@@ -29,7 +29,7 @@ pub mod supply_info;
 
 pub mod note_encryption;
 
-pub mod orchard_flavors;
+pub mod orchard_flavor;
 
 pub mod primitives;
 mod spec;

src/note_encryption/compact_action.rs

@@ -1,7 +1,5 @@
 //! Defines actions for Orchard shielded outputs and compact action for light clients.
 
-// FIXME: move to upper (src) folder?
-
 use std::fmt;
 
 use zcash_note_encryption_zsa::{EphemeralKeyBytes, ShieldedOutput};

src/note_encryption/domain.rs

@@ -46,10 +46,10 @@ const ZSA_ASSET_SIZE: usize = 32;
 pub(super) const COMPACT_NOTE_SIZE_ZSA: usize = COMPACT_NOTE_SIZE_VANILLA + ZSA_ASSET_SIZE;
 
 /// The version byte for Vanilla.
-pub(super) const NOTE_VERSION_BYTE_VANILLA: u8 = 0x02;
+pub(super) const NOTE_VERSION_BYTE_V2: u8 = 0x02;
 
 /// The version byte for ZSA.
-pub(super) const NOTE_VERSION_BYTE_ZSA: u8 = 0x03;
+pub(super) const NOTE_VERSION_BYTE_V3: u8 = 0x03;
 
 pub(super) type Memo = [u8; MEMO_SIZE];
 
@@ -83,7 +83,7 @@ pub(super) fn prf_ock_orchard(
 /// Returns `Some(u8)` if the version is recognized, otherwise `None`.
 pub(super) fn parse_note_version(plaintext: &[u8]) -> Option<u8> {
     plaintext.first().and_then(|version| match *version {
-        NOTE_VERSION_BYTE_VANILLA | NOTE_VERSION_BYTE_ZSA => Some(*version),
+        NOTE_VERSION_BYTE_V2 | NOTE_VERSION_BYTE_V3 => Some(*version),
         _ => None,
     })
 }
@@ -123,8 +123,8 @@ where
     let recipient = Address::from_parts(diversifier, pk_d);
 
     let asset = match parse_note_version(plaintext.as_ref())? {
-        NOTE_VERSION_BYTE_VANILLA => AssetBase::native(),
-        NOTE_VERSION_BYTE_ZSA => {
+        NOTE_VERSION_BYTE_V2 => AssetBase::native(),
+        NOTE_VERSION_BYTE_V3 => {
             let bytes = plaintext.as_ref()[COMPACT_NOTE_SIZE_VANILLA..COMPACT_NOTE_SIZE_ZSA]
                 .try_into()
                 .unwrap();

src/note_encryption/orchard_domain.rs

@@ -25,8 +25,6 @@ impl<const N: usize> AsMut<[u8]> for NoteBytesData<N> {
     }
 }
 
-// FIXME: consider implementing and using TryFrom instead (see also review comments in
-// zcash_note_encryption PR)
 impl<const N: usize> From<&[u8]> for NoteBytesData<N> {
     fn from(s: &[u8]) -> Self {
         Self(s.try_into().unwrap())

src/note_encryption/orchard_domain_vanilla.rs

@@ -1,10 +1,10 @@
 //! This module implements the note encryption logic specific for the `OrchardVanilla` flavor.
 
-use crate::{note::Note, orchard_flavors::OrchardVanilla};
+use crate::{note::Note, orchard_flavor::OrchardVanilla};
 
 use super::{
     domain::{
-        build_base_note_plaintext_bytes, Memo, COMPACT_NOTE_SIZE_VANILLA, NOTE_VERSION_BYTE_VANILLA,
+        build_base_note_plaintext_bytes, Memo, COMPACT_NOTE_SIZE_VANILLA, NOTE_VERSION_BYTE_V2,
     },
     orchard_domain::{NoteBytesData, OrchardDomainCommon},
 };
@@ -18,7 +18,7 @@ impl OrchardDomainCommon for OrchardVanilla {
     type CompactNoteCiphertextBytes = NoteBytesData<{ Self::COMPACT_NOTE_SIZE }>;
 
     fn build_note_plaintext_bytes(note: &Note, memo: &Memo) -> Self::NotePlaintextBytes {
-        let mut np = build_base_note_plaintext_bytes(NOTE_VERSION_BYTE_VANILLA, note);
+        let mut np = build_base_note_plaintext_bytes(NOTE_VERSION_BYTE_V2, note);
 
         np[COMPACT_NOTE_SIZE_VANILLA..].copy_from_slice(memo);
 
@@ -48,7 +48,7 @@ mod tests {
             testing::arb_native_note, AssetBase, ExtractedNoteCommitment, Note, Nullifier,
             RandomSeed, Rho, TransmittedNoteCiphertext,
         },
-        orchard_flavors::OrchardVanilla,
+        orchard_flavor::OrchardVanilla,
         primitives::redpallas,
         value::{NoteValue, ValueCommitment},
     };
@@ -57,7 +57,7 @@ mod tests {
         compact_action::CompactAction,
         domain::{
             parse_note_plaintext_without_memo, parse_note_version, prf_ock_orchard,
-            NOTE_VERSION_BYTE_VANILLA,
+            NOTE_VERSION_BYTE_V2,
         },
         orchard_domain::{NoteBytesData, OrchardDomain},
     };
@@ -81,7 +81,7 @@ mod tests {
 
             // Decode.
             let domain = OrchardDomainVanilla::for_rho(rho);
-            let parsed_version = parse_note_version(plaintext.as_ref()).unwrap();
+            let version = parse_note_version(plaintext.as_ref()).unwrap();
             let (compact, parsed_memo) = domain.extract_memo(&plaintext);
 
             let (parsed_note, parsed_recipient) = parse_note_plaintext_without_memo(rho, &compact,
@@ -95,7 +95,7 @@ mod tests {
             assert_eq!(parsed_note, note);
             assert_eq!(parsed_recipient, note.recipient());
             assert_eq!(&parsed_memo, memo);
-            assert_eq!(parsed_version, NOTE_VERSION_BYTE_VANILLA);
+            assert_eq!(version, NOTE_VERSION_BYTE_V2);
         }
     }