From 4f27b6bac36469070d2e034e94e44e3b3953d6d3 Mon Sep 17 00:00:00 2001 From: Hazel OHearn Date: Thu, 1 Dec 2022 16:51:04 -0400 Subject: [PATCH 1/8] Add concrete error types for add_spend and add_output --- CHANGELOG.md | 5 +++++ src/builder.rs | 29 +++++++++++++++++++++-------- 2 files changed, 26 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3ebdf382..b9f19532 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,9 +26,14 @@ and this project adheres to Rust's notion of - impls of `Eq` for: - `orchard::zip32::ChildIndex` - `orchard::value::ValueSum` +- `orchard::builder::Builder:` + - `SpendError` + - `OutputsDisabled` ### Changed - Migrated to `zcash_note_encryption 0.2`. +- `orchard::builder::Builder::{add_spend, add_output}` now use + concrete error types instead of `&'static str`s. ## [0.2.0] - 2022-06-24 ### Added diff --git a/src/builder.rs b/src/builder.rs index 9cbc2a5f..90c4af75 100644 --- a/src/builder.rs +++ b/src/builder.rs @@ -43,6 +43,20 @@ pub enum Error { DuplicateSignature, } +/// An error type for adding a spend to the builder. +#[derive(Debug)] +pub enum SpendError { + /// Spends aren't enabled for this builder. + SpendsDisabled, + /// The anchor provided to this builder doesn't match the merkle path used to add a spend. + AnchorMismatch, + /// The full viewing key provided didn't match the note provided + FvkMismatch, +} + +/// The only error that can occur here is if outputs are disabled for this builder. +pub type OutputsDisabled = (); + impl From for Error { fn from(e: halo2_proofs::plonk::Error) -> Self { Error::Proof(e) @@ -243,23 +257,22 @@ impl Builder { fvk: FullViewingKey, note: Note, merkle_path: MerklePath, - ) -> Result<(), &'static str> { + ) -> Result<(), SpendError> { if !self.flags.spends_enabled() { - return Err("Spends are not enabled for this builder"); + return Err(SpendError::SpendsDisabled); } // Consistency check: all anchors must be equal. let cm = note.commitment(); - let path_root: Anchor = - >::from(merkle_path.root(cm.into())).ok_or("Derived the bottom anchor")?; + let path_root = merkle_path.root(cm.into()); if path_root != self.anchor { - return Err("All anchors must be equal."); + return Err(SpendError::AnchorMismatch); } // Check if note is internal or external. let scope = fvk .scope_for_address(¬e.recipient()) - .ok_or("FullViewingKey does not correspond to the given note")?; + .ok_or(SpendError::FvkMismatch)?; self.spends.push(SpendInfo { dummy_sk: None, @@ -279,9 +292,9 @@ impl Builder { recipient: Address, value: NoteValue, memo: Option<[u8; 512]>, - ) -> Result<(), &'static str> { + ) -> Result<(), OutputsDisabled> { if !self.flags.outputs_enabled() { - return Err("Outputs are not enabled for this builder"); + return Err(()); } self.recipients.push(RecipientInfo { From 8c2326cdacc781c5d13e444d81510b23df7ef13e Mon Sep 17 00:00:00 2001 From: Hazel OHearn Date: Thu, 1 Dec 2022 16:58:26 -0400 Subject: [PATCH 2/8] Error -> BuildError --- CHANGELOG.md | 2 ++ src/builder.rs | 37 ++++++++++++++++++++----------------- 2 files changed, 22 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b9f19532..0c68b983 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,8 @@ and this project adheres to Rust's notion of - Migrated to `zcash_note_encryption 0.2`. - `orchard::builder::Builder::{add_spend, add_output}` now use concrete error types instead of `&'static str`s. +- `orchard::builder::Error` is now `BuildError` to differentiate from + new error types ## [0.2.0] - 2022-06-24 ### Added diff --git a/src/builder.rs b/src/builder.rs index 90c4af75..48348e6b 100644 --- a/src/builder.rs +++ b/src/builder.rs @@ -28,7 +28,7 @@ const MIN_ACTIONS: usize = 2; /// An error type for the kinds of errors that can occur during bundle construction. #[derive(Debug)] -pub enum Error { +pub enum BuildError { /// A bundle could not be built because required signatures were missing. MissingSignatures, /// An error occurred in the process of producing a proof for a bundle. @@ -57,15 +57,15 @@ pub enum SpendError { /// The only error that can occur here is if outputs are disabled for this builder. pub type OutputsDisabled = (); -impl From for Error { +impl From for BuildError { fn from(e: halo2_proofs::plonk::Error) -> Self { - Error::Proof(e) + BuildError::Proof(e) } } -impl From for Error { +impl From for BuildError { fn from(e: value::OverflowError) -> Self { - Error::ValueSum(e) + BuildError::ValueSum(e) } } @@ -339,7 +339,7 @@ impl Builder { pub fn build>( mut self, mut rng: impl RngCore, - ) -> Result, V>, Error> { + ) -> Result, V>, BuildError> { // Pair up the spends and recipients, extending with dummy values as necessary. let pre_actions: Vec<_> = { let num_spends = self.spends.len(); @@ -384,8 +384,8 @@ impl Builder { .ok_or(OverflowError)?; let result_value_balance: V = i64::try_from(value_balance) - .map_err(Error::ValueSum) - .and_then(|i| V::try_from(i).map_err(|_| Error::ValueSum(value::OverflowError)))?; + .map_err(BuildError::ValueSum) + .and_then(|i| V::try_from(i).map_err(|_| BuildError::ValueSum(value::OverflowError)))?; // Compute the transaction binding signing key. let bsk = pre_actions @@ -460,7 +460,7 @@ impl Bundle, V> { self, pk: &ProvingKey, mut rng: impl RngCore, - ) -> Result, V>, Error> { + ) -> Result, V>, BuildError> { let instances: Vec<_> = self .actions() .iter() @@ -535,10 +535,10 @@ pub enum MaybeSigned { } impl MaybeSigned { - fn finalize(self) -> Result, Error> { + fn finalize(self) -> Result, BuildError> { match self { Self::Signature(sig) => Ok(sig), - _ => Err(Error::MissingSignatures), + _ => Err(BuildError::MissingSignatures), } } } @@ -582,7 +582,7 @@ impl Bundle, V> { mut rng: R, sighash: [u8; 32], signing_keys: &[SpendAuthorizingKey], - ) -> Result, Error> { + ) -> Result, BuildError> { signing_keys .iter() .fold(self.prepare(&mut rng, sighash), |partial, ask| { @@ -621,11 +621,14 @@ impl Bundle, V> { pub fn append_signatures( self, signatures: &[redpallas::Signature], - ) -> Result { + ) -> Result { signatures.iter().try_fold(self, Self::append_signature) } - fn append_signature(self, signature: &redpallas::Signature) -> Result { + fn append_signature( + self, + signature: &redpallas::Signature, + ) -> Result { let mut signature_valid_for = 0usize; let bundle = self.map_authorization( &mut signature_valid_for, @@ -645,9 +648,9 @@ impl Bundle, V> { |_, partial| partial, ); match signature_valid_for { - 0 => Err(Error::InvalidExternalSignature), + 0 => Err(BuildError::InvalidExternalSignature), 1 => Ok(bundle), - _ => Err(Error::DuplicateSignature), + _ => Err(BuildError::DuplicateSignature), } } } @@ -656,7 +659,7 @@ impl Bundle, V> { /// Finalizes this bundle, enabling it to be included in a transaction. /// /// Returns an error if any signatures are missing. - pub fn finalize(self) -> Result, Error> { + pub fn finalize(self) -> Result, BuildError> { self.try_map_authorization( &mut (), |_, _, maybe| maybe.finalize(), From 6e8e2734e166857b7557a8b22fdcc3e3330c361f Mon Sep 17 00:00:00 2001 From: Hazel OHearn Date: Thu, 1 Dec 2022 17:01:10 -0400 Subject: [PATCH 3/8] Error -> BuildError, add PartialEq, Eq to SpendError --- src/builder.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/builder.rs b/src/builder.rs index 48348e6b..d3f4332f 100644 --- a/src/builder.rs +++ b/src/builder.rs @@ -44,7 +44,7 @@ pub enum BuildError { } /// An error type for adding a spend to the builder. -#[derive(Debug)] +#[derive(Debug, PartialEq, Eq)] pub enum SpendError { /// Spends aren't enabled for this builder. SpendsDisabled, From bf9ff1dae6e0fb0cdaa89fd04b98aa3aa1f4c446 Mon Sep 17 00:00:00 2001 From: Hazel OHearn Date: Wed, 7 Dec 2022 17:01:33 -0400 Subject: [PATCH 4/8] Fix changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0c68b983..5891da00 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,7 +26,7 @@ and this project adheres to Rust's notion of - impls of `Eq` for: - `orchard::zip32::ChildIndex` - `orchard::value::ValueSum` -- `orchard::builder::Builder:` +- `orchard::builder`: - `SpendError` - `OutputsDisabled` From 9807e325d7e89392dbb663ce6d067d2eea8dcb80 Mon Sep 17 00:00:00 2001 From: Hazel OHearn Date: Thu, 8 Dec 2022 11:00:50 -0400 Subject: [PATCH 5/8] Fix changelog --- CHANGELOG.md | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5891da00..261ea5dc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,16 @@ and this project adheres to Rust's notion of [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] +### Added +- `orchard::builder`: + - `SpendError` + - `OutputsDisabled` + +### Changed +- `orchard::builder::Builder::{add_spend, add_output}` now use + concrete error types instead of `&'static str`s. +- `orchard::builder::Error` is now `BuildError` to differentiate from + new error types ## [0.3.0] - 2022-10-19 ### Added @@ -26,16 +36,9 @@ and this project adheres to Rust's notion of - impls of `Eq` for: - `orchard::zip32::ChildIndex` - `orchard::value::ValueSum` -- `orchard::builder`: - - `SpendError` - - `OutputsDisabled` ### Changed - Migrated to `zcash_note_encryption 0.2`. -- `orchard::builder::Builder::{add_spend, add_output}` now use - concrete error types instead of `&'static str`s. -- `orchard::builder::Error` is now `BuildError` to differentiate from - new error types ## [0.2.0] - 2022-06-24 ### Added From 938b1228140783f01b05563585ffa26d11eacc99 Mon Sep 17 00:00:00 2001 From: Gygaxis Vainhardt Date: Thu, 8 Dec 2022 16:35:49 -0400 Subject: [PATCH 6/8] Replace type alias with zero-size struct Co-authored-by: Daira Hopwood --- src/builder.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/builder.rs b/src/builder.rs index d3f4332f..f69a3cb6 100644 --- a/src/builder.rs +++ b/src/builder.rs @@ -55,7 +55,8 @@ pub enum SpendError { } /// The only error that can occur here is if outputs are disabled for this builder. -pub type OutputsDisabled = (); +#[derive(Debug, PartialEq, Eq)] +pub struct OutputsDisabled; impl From for BuildError { fn from(e: halo2_proofs::plonk::Error) -> Self { From 4296860a862b587d60cac8e670101e523717aec4 Mon Sep 17 00:00:00 2001 From: Gygaxis Vainhardt Date: Thu, 8 Dec 2022 16:36:54 -0400 Subject: [PATCH 7/8] Partially apply suggestions from code review Co-authored-by: str4d --- CHANGELOG.md | 9 +++++---- src/builder.rs | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 261ea5dc..75ab53e9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,10 +12,11 @@ and this project adheres to Rust's notion of - `OutputsDisabled` ### Changed -- `orchard::builder::Builder::{add_spend, add_output}` now use - concrete error types instead of `&'static str`s. -- `orchard::builder::Error` is now `BuildError` to differentiate from - new error types +- `orchard::builder`: + - `Builder::{add_spend, add_output}` now use concrete error types instead of + `&'static str`s. + - `Error` has been renamed to `BuildError` to differentiate from new error + types. ## [0.3.0] - 2022-10-19 ### Added diff --git a/src/builder.rs b/src/builder.rs index f69a3cb6..847a3d14 100644 --- a/src/builder.rs +++ b/src/builder.rs @@ -295,7 +295,7 @@ impl Builder { memo: Option<[u8; 512]>, ) -> Result<(), OutputsDisabled> { if !self.flags.outputs_enabled() { - return Err(()); + return Err(OutputsDisabled); } self.recipients.push(RecipientInfo { From e466d7b523678379680553bb220b49314a90f3ef Mon Sep 17 00:00:00 2001 From: Hazel OHearn Date: Thu, 8 Dec 2022 16:58:30 -0400 Subject: [PATCH 8/8] Add Display and Error impls for error types --- CHANGELOG.md | 3 ++- src/builder.rs | 43 ++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 42 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 75ab53e9..b66e0f42 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to Rust's notion of ### Added - `orchard::builder`: - `SpendError` - - `OutputsDisabled` + - `OutputError` ### Changed - `orchard::builder`: @@ -17,6 +17,7 @@ and this project adheres to Rust's notion of `&'static str`s. - `Error` has been renamed to `BuildError` to differentiate from new error types. + - `BuildError` now implements `std::error::Error` and `std::fmt::Display`. ## [0.3.0] - 2022-10-19 ### Added diff --git a/src/builder.rs b/src/builder.rs index 847a3d14..ee5203e6 100644 --- a/src/builder.rs +++ b/src/builder.rs @@ -2,6 +2,7 @@ use core::fmt; use core::iter; +use std::fmt::Display; use ff::Field; use nonempty::NonEmpty; @@ -43,6 +44,21 @@ pub enum BuildError { DuplicateSignature, } +impl Display for BuildError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + use BuildError::*; + match self { + MissingSignatures => f.write_str("Required signatures were missing during build"), + Proof(e) => f.write_str(&format!("Could not create proof: {}", e.to_string())), + ValueSum(_) => f.write_str("Overflow occured during value construction"), + InvalidExternalSignature => f.write_str("External signature was invalid"), + DuplicateSignature => f.write_str("Signature valid for more than one input"), + } + } +} + +impl std::error::Error for BuildError {} + /// An error type for adding a spend to the builder. #[derive(Debug, PartialEq, Eq)] pub enum SpendError { @@ -54,9 +70,30 @@ pub enum SpendError { FvkMismatch, } +impl Display for SpendError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + use SpendError::*; + f.write_str(match self { + SpendsDisabled => "Spends are not enabled for this builder", + AnchorMismatch => "All anchors must be equal.", + FvkMismatch => "FullViewingKey does not correspond to the given note", + }) + } +} + +impl std::error::Error for SpendError {} + /// The only error that can occur here is if outputs are disabled for this builder. #[derive(Debug, PartialEq, Eq)] -pub struct OutputsDisabled; +pub struct OutputError; + +impl Display for OutputError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str("Outputs are not enabled for this builder") + } +} + +impl std::error::Error for OutputError {} impl From for BuildError { fn from(e: halo2_proofs::plonk::Error) -> Self { @@ -293,9 +330,9 @@ impl Builder { recipient: Address, value: NoteValue, memo: Option<[u8; 512]>, - ) -> Result<(), OutputsDisabled> { + ) -> Result<(), OutputError> { if !self.flags.outputs_enabled() { - return Err(OutputsDisabled); + return Err(OutputError); } self.recipients.push(RecipientInfo {