From fc2f811607d44142b2d502ccb266a7d9cb339fb5 Mon Sep 17 00:00:00 2001 From: Dmitry Demin Date: Wed, 19 Apr 2023 13:44:04 +0200 Subject: [PATCH 1/8] Add tracking for supply info inside verify_issue_bundle --- src/issuance.rs | 224 ++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 180 insertions(+), 44 deletions(-) diff --git a/src/issuance.rs b/src/issuance.rs index f3da1523b..11df62e53 100644 --- a/src/issuance.rs +++ b/src/issuance.rs @@ -2,21 +2,21 @@ use blake2b_simd::Hash as Blake2bHash; use nonempty::NonEmpty; use rand::{CryptoRng, RngCore}; -use std::collections::HashSet; +use std::collections::{HashMap, HashSet}; use std::fmt; use crate::bundle::commitments::{hash_issue_bundle_auth_data, hash_issue_bundle_txid_data}; use crate::issuance::Error::{ IssueActionAlreadyFinalized, IssueActionIncorrectNoteType, IssueActionNotFound, IssueActionPreviouslyFinalizedNoteType, IssueBundleIkMismatchNoteType, - IssueBundleInvalidSignature, WrongAssetDescSize, + IssueBundleInvalidSignature, ValueSumOverflow, WrongAssetDescSize, }; use crate::keys::{IssuanceAuthorizingKey, IssuanceValidatingKey}; use crate::note::asset_base::is_asset_desc_of_valid_size; use crate::note::{AssetBase, Nullifier}; use crate::primitives::redpallas::Signature; -use crate::value::NoteValue; +use crate::value::{NoteValue, ValueSum}; use crate::{ primitives::redpallas::{self, SpendAuth}, Address, Note, @@ -83,26 +83,37 @@ impl IssueAction { self.finalize } - /// Return the `AssetBase` if the provided `ik` is used to derive the `asset_id` for **all** internal notes. + /// Return the (`AssetBase`, `ValueSum`) tuple if the provided `ik` is used to derive + /// the `asset_id` for **all** internal notes. + // FIXME: consider renaming this function as it not only validates assets in notes + // but also calculates the sum of the note values. fn are_note_asset_ids_derived_correctly( &self, ik: &IssuanceValidatingKey, - ) -> Result { - match self - .notes - .iter() - .try_fold(self.notes().head.asset(), |asset, ¬e| { - // Fail if not all note types are equal - note.asset() - .eq(&asset) - .then(|| asset) - .ok_or(IssueActionIncorrectNoteType) - }) { - Ok(asset) => asset // check that the asset was properly derived. - .eq(&AssetBase::derive(ik, &self.asset_desc)) - .then(|| asset) - .ok_or(IssueBundleIkMismatchNoteType), - Err(e) => Err(e), + ) -> Result<(AssetBase, ValueSum), Error> { + // Calculate the value of the asset as a sum of values of all its notes + // and ensure all note types are equal + let (asset, value_sum) = self.notes.iter().try_fold( + (self.notes().head.asset(), ValueSum::zero()), + |(asset, value_sum), ¬e| { + // Update asset value (fail if not all note types are equal + // or if overflow occured) + if note.asset().eq(&asset) { + let updated_value_sum = + (value_sum + i128::from(note.value())).ok_or(ValueSumOverflow)?; + Ok((asset, updated_value_sum)) + } else { + Err(IssueActionIncorrectNoteType) + } + }, + )?; + + // Return the asset and its value (check that the asset was properly + // derived before returning) + if asset.eq(&AssetBase::derive(ik, &self.asset_desc)) { + Ok((asset, value_sum)) + } else { + Err(IssueBundleIkMismatchNoteType) } } } @@ -365,44 +376,74 @@ impl IssueBundle { /// * `AssetBase` for the `IssueAction` has not been previously finalized. /// * For each `Note` inside an `IssueAction`: /// * All notes have the same, correct `AssetBase`. +/// +/// Returns a `Result` with supply info stored in a `HashMap`. The `HashMap` uses +/// `AssetBase` as the key, and a tuple as the value. The tuple contains a `ValueSum` +/// (representing the total value of all notes for the asset) and a `bool` indicating +/// whether the asset is finalized. +/// +/// Possible errors: +/// +/// * `IssueBundleInvalidSignature`: This error occurs if the signature verification +/// for the provided `sighash` fails. +/// * `WrongAssetDescSize`: This error is raised if the asset description size for any +/// asset in the bundle is incorrect. +/// * `IssueActionPreviouslyFinalizedNoteType`: This error occurs if the asset has already been +/// finalized (inserted into the `finalized` collection). +/// * `IssueActionIncorrectNoteType`: This error occurs if any note has an incorrect note type. +/// * `ValueSumOverflow**: This error occurs if an overflow happens during the calculation of +/// the value sum for the notes in the asset. +/// * `IssueBundleIkMismatchNoteType`: This error is raised if the `AssetBase` derived from +/// the `ik` (Issuance Validating Key) and the `asset_desc` (Asset Description) does not match +/// the expected `AssetBase`. pub fn verify_issue_bundle( bundle: &IssueBundle, sighash: [u8; 32], finalized: &mut HashSet, // The finalization set. -) -> Result<(), Error> { +) -> Result, Error> { if let Err(e) = bundle.ik.verify(&sighash, &bundle.authorization.signature) { return Err(IssueBundleInvalidSignature(e)); }; - let s = &mut HashSet::::new(); - - let newly_finalized = bundle - .actions() - .iter() - .try_fold(s, |newly_finalized, action| { + let supply_info = bundle.actions().iter().try_fold( + HashMap::::new(), + // Rust uses move semantics here so supply_info is not copied but moved + |mut supply_info, action| { if !is_asset_desc_of_valid_size(action.asset_desc()) { return Err(WrongAssetDescSize); } - // Fail if any note in the IssueAction has incorrect note type. - let asset = action.are_note_asset_ids_derived_correctly(bundle.ik())?; - - // Fail if the asset was previously finalized. - if finalized.contains(&asset) || newly_finalized.contains(&asset) { - return Err(IssueActionPreviouslyFinalizedNoteType(asset)); - } - - // Add to the finalization set, if needed. - if action.is_finalized() { - newly_finalized.insert(asset); + // Fail if any note in the IssueAction has an incorrect note type or + // an overflow occurred during value_sum calculation + let (asset, value_sum) = action.are_note_asset_ids_derived_correctly(bundle.ik())?; + + // Attempt to insert the asset into the supply info collection (fails if the asset was + // previously finalized or inserted into the supply info). + // + // FIXME: Use try_insert instead of insert when it's stabilized. + // + // FIXME: Determine the proper behavior if supply_info already contains the + // asset, but is_finalized for it is false. Currently, it will be simply overwritten + // by the supply_info.insert call (with new value_sum and is_finalized values). + if finalized.contains(&asset) + || supply_info + .insert(asset, (value_sum, action.is_finalized())) + .map_or(false, |(_, is_finalized)| is_finalized) + { + Err(IssueActionPreviouslyFinalizedNoteType(asset)) + } else { + Ok(supply_info) } + }, + )?; - // Proceed with the new finalization set. - Ok(newly_finalized) - })?; + finalized.extend( + supply_info + .iter() + .filter_map(|(asset, (_, is_finalized))| is_finalized.then(|| asset)), + ); - finalized.extend(newly_finalized.iter()); - Ok(()) + Ok(supply_info) } /// Errors produced during the issuance process @@ -424,6 +465,9 @@ pub enum Error { IssueBundleInvalidSignature(reddsa::Error), /// The provided `NoteType` has been previously finalized. IssueActionPreviouslyFinalizedNoteType(AssetBase), + + /// Overflow error occurred while calculating the value of the asset + ValueSumOverflow, } impl std::error::Error for Error {} @@ -458,6 +502,12 @@ impl fmt::Display for Error { IssueActionPreviouslyFinalizedNoteType(_) => { write!(f, "the provided `NoteType` has been previously finalized") } + ValueSumOverflow => { + write!( + f, + "overflow error occurred while calculating the value of the asset" + ) + } } } } @@ -475,7 +525,7 @@ mod tests { FullViewingKey, IssuanceAuthorizingKey, IssuanceValidatingKey, Scope, SpendingKey, }; use crate::note::{AssetBase, Nullifier}; - use crate::value::NoteValue; + use crate::value::{NoteValue, ValueSum}; use crate::{Address, Note}; use nonempty::NonEmpty; use rand::rngs::OsRng; @@ -812,6 +862,92 @@ mod tests { assert_eq!(prev_finalized.len(), 1); } + #[test] + fn issue_bundle_verify_with_supply_info() { + let (rng, isk, ik, recipient, sighash) = setup_params(); + + let mut bundle = IssueBundle::new(ik.clone()); + + let asset1_desc = "Verify with supply info 1"; + let asset2_desc = "Verify with supply info 2"; + let asset3_desc = "Verify with supply info 3"; + + let asset1_base = AssetBase::derive(&ik, &String::from(asset1_desc)); + let asset2_base = AssetBase::derive(&ik, &String::from(asset2_desc)); + let asset3_base = AssetBase::derive(&ik, &String::from(asset3_desc)); + + bundle + .add_recipient( + String::from(asset1_desc), + recipient, + NoteValue::from_raw(7), + false, + rng, + ) + .unwrap(); + + bundle + .add_recipient( + String::from(asset1_desc), + recipient, + NoteValue::from_raw(8), + true, + rng, + ) + .unwrap(); + + bundle + .add_recipient( + String::from(asset2_desc), + recipient, + NoteValue::from_raw(10), + true, + rng, + ) + .unwrap(); + + bundle + .add_recipient( + String::from(asset3_desc), + recipient, + NoteValue::from_raw(5), + false, + rng, + ) + .unwrap(); + + let signed = bundle.prepare(sighash).sign(rng, &isk).unwrap(); + + let prev_finalized = &mut HashSet::new(); + + let res = verify_issue_bundle(&signed, sighash, prev_finalized); + + assert!(res.is_ok()); + + assert_eq!(prev_finalized.len(), 2); + + assert!(prev_finalized.contains(&asset1_base)); + assert!(prev_finalized.contains(&asset2_base)); + assert!(!prev_finalized.contains(&asset3_base)); + + let supply_info = res.unwrap(); + + assert_eq!(supply_info.len(), 3); + + assert_eq!( + supply_info.get(&asset1_base), + Some(&(ValueSum::from_raw(15), true)) + ); + assert_eq!( + supply_info.get(&asset2_base), + Some(&(ValueSum::from_raw(10), true)) + ); + assert_eq!( + supply_info.get(&asset3_base), + Some(&(ValueSum::from_raw(5), false)) + ); + } + #[test] fn issue_bundle_verify_fail_previously_finalized() { let (rng, isk, ik, recipient, sighash) = setup_params(); From dc84dbf45831b724f9932125370a5419b80be48c Mon Sep 17 00:00:00 2001 From: Dmitry Demin Date: Sat, 22 Apr 2023 21:08:18 +0200 Subject: [PATCH 2/8] Resolve review remarks (fix issues, add docs, tests for compute_new_supply etc.) --- src/issuance.rs | 345 +++++++++++++++++++++++++++++++++++------------- 1 file changed, 250 insertions(+), 95 deletions(-) diff --git a/src/issuance.rs b/src/issuance.rs index 11df62e53..cd9312234 100644 --- a/src/issuance.rs +++ b/src/issuance.rs @@ -7,8 +7,8 @@ use std::fmt; use crate::bundle::commitments::{hash_issue_bundle_auth_data, hash_issue_bundle_txid_data}; use crate::issuance::Error::{ - IssueActionAlreadyFinalized, IssueActionIncorrectNoteType, IssueActionNotFound, - IssueActionPreviouslyFinalizedNoteType, IssueBundleIkMismatchNoteType, + IssueActionAlreadyFinalized, IssueActionIncorrectAssetBase, IssueActionNotFound, + IssueActionPreviouslyFinalizedAssetBase, IssueBundleIkMismatchAssetBase, IssueBundleInvalidSignature, ValueSumOverflow, WrongAssetDescSize, }; use crate::keys::{IssuanceAuthorizingKey, IssuanceValidatingKey}; @@ -33,6 +33,46 @@ pub struct IssueBundle { authorization: T, } +/// Represents the amount of an asset and its finalization status. +#[derive(Debug, Clone)] +#[cfg_attr(test, derive(PartialEq))] +pub struct AssetSupply { + /// The amount of the asset. + amount: ValueSum, + /// Whether or not the asset is finalized. + is_finalized: bool, +} + +impl AssetSupply { + /// Creates a new AssetSupply instance with the given amount and finalization status. + pub fn new(amount: ValueSum, is_finalized: bool) -> Self { + Self { + amount, + is_finalized, + } + } +} + +/// Contains information about the supply of assets. +#[derive(Debug, Clone, Default)] +pub struct SupplyInfo { + /// A map of asset bases to their respective supply information. + assets: HashMap, +} + +impl SupplyInfo { + /// Attempts to insert an asset's supply information into the supply info map. + /// Returns true on success or false if the supply for the asset was already inserted and finalized. + pub fn try_insert_asset_supply(&mut self, asset: AssetBase, supply: AssetSupply) -> bool { + // FIXME: Determine the proper behavior if `assets` already contains the + // asset, but is_finalized for it is false. Currently, it will be simply overwritten + // by the supply_info.insert call (with new value_sum and is_finalized values). + self.assets + .insert(asset, supply) + .map_or(true, |supply| !supply.is_finalized) + } +} + /// An issue action applied to the global ledger. /// /// Externally, this creates new zsa notes (adding a commitment to the global ledger). @@ -83,38 +123,58 @@ impl IssueAction { self.finalize } - /// Return the (`AssetBase`, `ValueSum`) tuple if the provided `ik` is used to derive - /// the `asset_id` for **all** internal notes. - // FIXME: consider renaming this function as it not only validates assets in notes - // but also calculates the sum of the note values. - fn are_note_asset_ids_derived_correctly( + /// Computes the new asset supply for an `IssueAction`. + /// + /// This function calculates the total value of the asset by summing the values of all its notes + /// and ensures that all note types are equal. It returns the asset and its supply as a tuple + /// (`AssetBase`, `AssetSupply`) or an error if the asset was not properly derived or an + /// overflow occurred during the supply amount calculation. + /// + /// # Arguments + /// + /// * `ik` - A reference to the `IssuanceValidatingKey` used for deriving the asset. + /// + /// # Returns + /// + /// A `Result` containing a tuple with an `AssetBase` and an `AssetSupply`, or an `Error`. + /// + /// # Errors + /// + /// This function may return an error in any of the following cases: + /// + /// * `IssueActionIncorrectAssetBase`: If the asset type of any note in the `IssueAction` is + /// not equal to the asset type of the first note. + /// + /// * `ValueSumOverflow`: If the total amount value of all notes in the `IssueAction` overflows. + /// + /// * `IssueBundleIkMismatchAssetBase`: If the provided `ik` is not used to derive the + /// `asset_id` for **all** internal notes. + fn compute_new_supply( &self, ik: &IssuanceValidatingKey, - ) -> Result<(AssetBase, ValueSum), Error> { + ) -> Result<(AssetBase, AssetSupply), Error> { // Calculate the value of the asset as a sum of values of all its notes // and ensure all note types are equal let (asset, value_sum) = self.notes.iter().try_fold( (self.notes().head.asset(), ValueSum::zero()), |(asset, value_sum), ¬e| { - // Update asset value (fail if not all note types are equal - // or if overflow occured) - if note.asset().eq(&asset) { - let updated_value_sum = - (value_sum + i128::from(note.value())).ok_or(ValueSumOverflow)?; - Ok((asset, updated_value_sum)) - } else { - Err(IssueActionIncorrectNoteType) - } + // All assets should have the same asset + note.asset() + .eq(&asset) + .then(|| ()) + .ok_or(IssueActionIncorrectAssetBase)?; + + // The total amount should not overflow + Ok((asset, (value_sum + note.value()).ok_or(ValueSumOverflow)?)) }, )?; - // Return the asset and its value (check that the asset was properly - // derived before returning) - if asset.eq(&AssetBase::derive(ik, &self.asset_desc)) { - Ok((asset, value_sum)) - } else { - Err(IssueBundleIkMismatchNoteType) - } + // Return the asset and its value (or an error if the asset was not properly + // derived) + asset + .eq(&AssetBase::derive(ik, &self.asset_desc)) + .then(|| Ok((asset, AssetSupply::new(value_sum, self.is_finalized())))) + .ok_or(IssueBundleIkMismatchAssetBase)? } } @@ -319,11 +379,9 @@ impl IssueBundle { let expected_ik: IssuanceValidatingKey = (isk).into(); // Make sure the `expected_ik` matches the `asset` for all notes. - self.actions.iter().try_for_each(|action| { - action - .are_note_asset_ids_derived_correctly(&expected_ik) - .map(|_| ()) // Transform Result into Result<(),Error)>. - })?; + self.actions + .iter() + .try_for_each(|action| action.compute_new_supply(&expected_ik).map(|_| ()))?; Ok(IssueBundle { ik: self.ik, @@ -377,70 +435,64 @@ impl IssueBundle { /// * For each `Note` inside an `IssueAction`: /// * All notes have the same, correct `AssetBase`. /// -/// Returns a `Result` with supply info stored in a `HashMap`. The `HashMap` uses -/// `AssetBase` as the key, and a tuple as the value. The tuple contains a `ValueSum` -/// (representing the total value of all notes for the asset) and a `bool` indicating -/// whether the asset is finalized. +// # Returns /// -/// Possible errors: +/// A Result containing a SupplyInfo struct, which stores supply information in a HashMap. +/// The HashMap uses AssetBase as the key, and an AssetSupply struct as the value. The +/// AssetSupply contains a ValueSum (representing the total value of all notes for the asset) +/// and a bool indicating whether the asset is finalized. +/// +/// # Errors /// /// * `IssueBundleInvalidSignature`: This error occurs if the signature verification /// for the provided `sighash` fails. /// * `WrongAssetDescSize`: This error is raised if the asset description size for any /// asset in the bundle is incorrect. -/// * `IssueActionPreviouslyFinalizedNoteType`: This error occurs if the asset has already been +/// * `IssueActionPreviouslyFinalizedAssetBase`: This error occurs if the asset has already been /// finalized (inserted into the `finalized` collection). -/// * `IssueActionIncorrectNoteType`: This error occurs if any note has an incorrect note type. -/// * `ValueSumOverflow**: This error occurs if an overflow happens during the calculation of +/// * `IssueActionIncorrectAssetBase`: This error occurs if any note has an incorrect note type. +/// * `ValueSumOverflow`: This error occurs if an overflow happens during the calculation of /// the value sum for the notes in the asset. -/// * `IssueBundleIkMismatchNoteType`: This error is raised if the `AssetBase` derived from +/// * `IssueBundleIkMismatchAssetBase`: This error is raised if the `AssetBase` derived from /// the `ik` (Issuance Validating Key) and the `asset_desc` (Asset Description) does not match /// the expected `AssetBase`. pub fn verify_issue_bundle( bundle: &IssueBundle, sighash: [u8; 32], finalized: &mut HashSet, // The finalization set. -) -> Result, Error> { +) -> Result { if let Err(e) = bundle.ik.verify(&sighash, &bundle.authorization.signature) { return Err(IssueBundleInvalidSignature(e)); }; - let supply_info = bundle.actions().iter().try_fold( - HashMap::::new(), - // Rust uses move semantics here so supply_info is not copied but moved - |mut supply_info, action| { - if !is_asset_desc_of_valid_size(action.asset_desc()) { - return Err(WrongAssetDescSize); - } + let supply_info = + bundle + .actions() + .iter() + .try_fold(SupplyInfo::default(), |mut supply_info, action| { + if !is_asset_desc_of_valid_size(action.asset_desc()) { + return Err(WrongAssetDescSize); + } - // Fail if any note in the IssueAction has an incorrect note type or - // an overflow occurred during value_sum calculation - let (asset, value_sum) = action.are_note_asset_ids_derived_correctly(bundle.ik())?; - - // Attempt to insert the asset into the supply info collection (fails if the asset was - // previously finalized or inserted into the supply info). - // - // FIXME: Use try_insert instead of insert when it's stabilized. - // - // FIXME: Determine the proper behavior if supply_info already contains the - // asset, but is_finalized for it is false. Currently, it will be simply overwritten - // by the supply_info.insert call (with new value_sum and is_finalized values). - if finalized.contains(&asset) - || supply_info - .insert(asset, (value_sum, action.is_finalized())) - .map_or(false, |(_, is_finalized)| is_finalized) - { - Err(IssueActionPreviouslyFinalizedNoteType(asset)) - } else { - Ok(supply_info) - } - }, - )?; + let (asset, supply) = action.compute_new_supply(bundle.ik())?; + + // Insert new supply into the supply info (fails if the asset was + // previously finalized or if the supply for the asset was already + // inserted and finalized) + if finalized.contains(&asset) + || (!supply_info.try_insert_asset_supply(asset, supply)) + { + Err(IssueActionPreviouslyFinalizedAssetBase(asset)) + } else { + Ok(supply_info) + } + })?; finalized.extend( supply_info + .assets .iter() - .filter_map(|(asset, (_, is_finalized))| is_finalized.then(|| asset)), + .filter_map(|(asset, supply)| supply.is_finalized.then(|| asset)), ); Ok(supply_info) @@ -453,18 +505,18 @@ pub enum Error { IssueActionAlreadyFinalized, /// The requested IssueAction not exists in the bundle. IssueActionNotFound, - /// Not all `NoteType`s are the same inside the action. - IssueActionIncorrectNoteType, + /// Not all `AssetBase`s are the same inside the action. + IssueActionIncorrectAssetBase, /// The provided `isk` and the driven `ik` does not match at least one note type. - IssueBundleIkMismatchNoteType, + IssueBundleIkMismatchAssetBase, /// `asset_desc` should be between 1 and 512 bytes. WrongAssetDescSize, /// Verification errors: /// Invalid signature. IssueBundleInvalidSignature(reddsa::Error), - /// The provided `NoteType` has been previously finalized. - IssueActionPreviouslyFinalizedNoteType(AssetBase), + /// The provided `AssetBase` has been previously finalized. + IssueActionPreviouslyFinalizedAssetBase(AssetBase), /// Overflow error occurred while calculating the value of the asset ValueSumOverflow, @@ -484,10 +536,10 @@ impl fmt::Display for Error { IssueActionNotFound => { write!(f, "the requested IssueAction not exists in the bundle.") } - IssueActionIncorrectNoteType => { - write!(f, "not all `NoteType`s are the same inside the action") + IssueActionIncorrectAssetBase => { + write!(f, "not all `AssetBase`s are the same inside the action") } - IssueBundleIkMismatchNoteType => { + IssueBundleIkMismatchAssetBase => { write!( f, "the provided `isk` and the driven `ik` does not match at least one note type" @@ -499,8 +551,8 @@ impl fmt::Display for Error { IssueBundleInvalidSignature(_) => { write!(f, "invalid signature") } - IssueActionPreviouslyFinalizedNoteType(_) => { - write!(f, "the provided `NoteType` has been previously finalized") + IssueActionPreviouslyFinalizedAssetBase(_) => { + write!(f, "the provided `AssetBase` has been previously finalized") } ValueSumOverflow => { write!( @@ -514,10 +566,10 @@ impl fmt::Display for Error { #[cfg(test)] mod tests { - use super::IssueBundle; + use super::{AssetSupply, IssueBundle}; use crate::issuance::Error::{ - IssueActionAlreadyFinalized, IssueActionIncorrectNoteType, IssueActionNotFound, - IssueActionPreviouslyFinalizedNoteType, IssueBundleIkMismatchNoteType, + IssueActionAlreadyFinalized, IssueActionIncorrectAssetBase, IssueActionNotFound, + IssueActionPreviouslyFinalizedAssetBase, IssueBundleIkMismatchAssetBase, IssueBundleInvalidSignature, WrongAssetDescSize, }; use crate::issuance::{verify_issue_bundle, IssueAction, Signed}; @@ -555,6 +607,109 @@ mod tests { (rng, isk, ik, recipient, sighash) } + fn setup_compute_new_supply_test_params( + asset_desc: &str, + note1_value: u64, + note2_value: u64, + note2_asset_desc: Option<&str>, // if None, both notes use the same asset + finalize: bool, + ) -> (IssuanceValidatingKey, AssetBase, IssueAction) { + let (mut rng, _, ik, recipient, _) = setup_params(); + + let asset = AssetBase::derive(&ik, asset_desc); + let note2_asset = note2_asset_desc.map_or(asset, |desc| AssetBase::derive(&ik, desc)); + + let note1 = Note::new( + recipient, + NoteValue::from_raw(note1_value), + asset, + Nullifier::dummy(&mut rng), + &mut rng, + ); + + let note2 = Note::new( + recipient, + NoteValue::from_raw(note2_value), + note2_asset, + Nullifier::dummy(&mut rng), + &mut rng, + ); + + ( + ik, + asset, + IssueAction::from_parts( + asset_desc.into(), + NonEmpty { + head: note1, + tail: vec![note2], + }, + finalize, + ), + ) + } + + #[test] + fn test_compute_new_supply_valid() { + let (ik, test_asset, action) = + setup_compute_new_supply_test_params("Asset 1", 10, 20, None, false); + + let result = action.compute_new_supply(&ik); + + assert!(result.is_ok()); + + let (asset, supply) = result.unwrap(); + + assert_eq!(asset, test_asset); + assert_eq!(supply.amount, ValueSum::from_raw(30)); + assert!(!supply.is_finalized); + } + + #[test] + fn test_compute_new_supply_finalized() { + let (ik, test_asset, action) = + setup_compute_new_supply_test_params("Asset 1", 10, 20, None, true); + + let result = action.compute_new_supply(&ik); + + assert!(result.is_ok()); + + let (asset, supply) = result.unwrap(); + + assert_eq!(asset, test_asset); + assert_eq!(supply.amount, ValueSum::from_raw(30)); + assert!(supply.is_finalized); + } + + // FIXME: Evaluate the feasibility of implementing this test. Note values are + // represented by the NoteValue type (u64), while the supply amount uses the + // ValueSum type (i128). Due to the difference in types, simulating a supply + // amount overflow (i.e., the sum of values of notes) is challenging. + #[test] + fn test_compute_new_supply_value_sum_overflow() {} + + #[test] + fn test_compute_new_supply_incorrect_asset_base() { + let (ik, _, action) = + setup_compute_new_supply_test_params("Asset 1", 10, 20, Some("Asset 2"), false); + + assert_eq!( + action.compute_new_supply(&ik), + Err(IssueActionIncorrectAssetBase) + ); + } + + #[test] + fn test_compute_new_supply_ik_mismatch_asset_base() { + let (_, _, action) = setup_compute_new_supply_test_params("Asset 1", 10, 20, None, false); + let (_, _, ik, _, _) = setup_params(); + + assert_eq!( + action.compute_new_supply(&ik), + Err(IssueBundleIkMismatchAssetBase) + ); + } + #[test] fn issue_bundle_basic() { let (rng, _, ik, recipient, _) = setup_params(); @@ -764,7 +919,7 @@ mod tests { .sign(rng, &wrong_isk) .expect_err("should not be able to sign"); - assert_eq!(err, IssueBundleIkMismatchNoteType); + assert_eq!(err, IssueBundleIkMismatchAssetBase); } #[test] @@ -805,7 +960,7 @@ mod tests { .sign(rng, &isk) .expect_err("should not be able to sign"); - assert_eq!(err, IssueActionIncorrectNoteType); + assert_eq!(err, IssueActionIncorrectAssetBase); } #[test] @@ -932,19 +1087,19 @@ mod tests { let supply_info = res.unwrap(); - assert_eq!(supply_info.len(), 3); + assert_eq!(supply_info.assets.len(), 3); assert_eq!( - supply_info.get(&asset1_base), - Some(&(ValueSum::from_raw(15), true)) + supply_info.assets.get(&asset1_base), + Some(&AssetSupply::new(ValueSum::from_raw(15), true)) ); assert_eq!( - supply_info.get(&asset2_base), - Some(&(ValueSum::from_raw(10), true)) + supply_info.assets.get(&asset2_base), + Some(&AssetSupply::new(ValueSum::from_raw(10), true)) ); assert_eq!( - supply_info.get(&asset3_base), - Some(&(ValueSum::from_raw(5), false)) + supply_info.assets.get(&asset3_base), + Some(&AssetSupply::new(ValueSum::from_raw(5), false)) ); } @@ -974,7 +1129,7 @@ mod tests { let finalized = verify_issue_bundle(&signed, sighash, prev_finalized); assert_eq!( finalized.unwrap_err(), - IssueActionPreviouslyFinalizedNoteType(final_type) + IssueActionPreviouslyFinalizedAssetBase(final_type) ); } @@ -1083,7 +1238,7 @@ mod tests { let prev_finalized = &mut HashSet::new(); let err = verify_issue_bundle(&signed, sighash, prev_finalized).unwrap_err(); - assert_eq!(err, IssueActionIncorrectNoteType); + assert_eq!(err, IssueActionIncorrectAssetBase); } #[test] @@ -1124,7 +1279,7 @@ mod tests { let prev_finalized = &mut HashSet::new(); let err = verify_issue_bundle(&signed, sighash, prev_finalized).unwrap_err(); - assert_eq!(err, IssueBundleIkMismatchNoteType); + assert_eq!(err, IssueBundleIkMismatchAssetBase); } #[test] From 10e7aa088561036d8c9b84b2db236237691a0ef0 Mon Sep 17 00:00:00 2001 From: Dmitry Demin Date: Mon, 24 Apr 2023 14:36:03 +0200 Subject: [PATCH 3/8] Resolve review remarks 2 (rename compute_new_supply to verify_supply, change add_supply behavior etc.) --- src/issuance.rs | 124 ++++++++++++++------------------------------- src/lib.rs | 1 + src/supply_info.rs | 99 ++++++++++++++++++++++++++++++++++++ 3 files changed, 137 insertions(+), 87 deletions(-) create mode 100644 src/supply_info.rs diff --git a/src/issuance.rs b/src/issuance.rs index cd9312234..50d6e5462 100644 --- a/src/issuance.rs +++ b/src/issuance.rs @@ -2,7 +2,7 @@ use blake2b_simd::Hash as Blake2bHash; use nonempty::NonEmpty; use rand::{CryptoRng, RngCore}; -use std::collections::{HashMap, HashSet}; +use std::collections::HashSet; use std::fmt; use crate::bundle::commitments::{hash_issue_bundle_auth_data, hash_issue_bundle_txid_data}; @@ -22,6 +22,8 @@ use crate::{ Address, Note, }; +use crate::supply_info::{AssetSupply, SupplyInfo}; + /// A bundle of actions to be applied to the ledger. #[derive(Debug, Clone)] pub struct IssueBundle { @@ -33,46 +35,6 @@ pub struct IssueBundle { authorization: T, } -/// Represents the amount of an asset and its finalization status. -#[derive(Debug, Clone)] -#[cfg_attr(test, derive(PartialEq))] -pub struct AssetSupply { - /// The amount of the asset. - amount: ValueSum, - /// Whether or not the asset is finalized. - is_finalized: bool, -} - -impl AssetSupply { - /// Creates a new AssetSupply instance with the given amount and finalization status. - pub fn new(amount: ValueSum, is_finalized: bool) -> Self { - Self { - amount, - is_finalized, - } - } -} - -/// Contains information about the supply of assets. -#[derive(Debug, Clone, Default)] -pub struct SupplyInfo { - /// A map of asset bases to their respective supply information. - assets: HashMap, -} - -impl SupplyInfo { - /// Attempts to insert an asset's supply information into the supply info map. - /// Returns true on success or false if the supply for the asset was already inserted and finalized. - pub fn try_insert_asset_supply(&mut self, asset: AssetBase, supply: AssetSupply) -> bool { - // FIXME: Determine the proper behavior if `assets` already contains the - // asset, but is_finalized for it is false. Currently, it will be simply overwritten - // by the supply_info.insert call (with new value_sum and is_finalized values). - self.assets - .insert(asset, supply) - .map_or(true, |supply| !supply.is_finalized) - } -} - /// An issue action applied to the global ledger. /// /// Externally, this creates new zsa notes (adding a commitment to the global ledger). @@ -123,12 +85,12 @@ impl IssueAction { self.finalize } - /// Computes the new asset supply for an `IssueAction`. + /// Verifies and computes the new asset supply for an `IssueAction`. /// - /// This function calculates the total value of the asset by summing the values of all its notes - /// and ensures that all note types are equal. It returns the asset and its supply as a tuple - /// (`AssetBase`, `AssetSupply`) or an error if the asset was not properly derived or an - /// overflow occurred during the supply amount calculation. + /// This function calculates the total value (supply) of the asset by summing the values + /// of all its notes and ensures that all note types are equal. It returns the asset and + /// its supply as a tuple (`AssetBase`, `AssetSupply`) or an error if the asset was not + /// properly derived or an overflow occurred during the supply amount calculation. /// /// # Arguments /// @@ -149,10 +111,7 @@ impl IssueAction { /// /// * `IssueBundleIkMismatchAssetBase`: If the provided `ik` is not used to derive the /// `asset_id` for **all** internal notes. - fn compute_new_supply( - &self, - ik: &IssuanceValidatingKey, - ) -> Result<(AssetBase, AssetSupply), Error> { + fn verify_supply(&self, ik: &IssuanceValidatingKey) -> Result<(AssetBase, AssetSupply), Error> { // Calculate the value of the asset as a sum of values of all its notes // and ensure all note types are equal let (asset, value_sum) = self.notes.iter().try_fold( @@ -169,8 +128,7 @@ impl IssueAction { }, )?; - // Return the asset and its value (or an error if the asset was not properly - // derived) + // Return the asset and its supply (or an error if the asset was not properly derived) asset .eq(&AssetBase::derive(ik, &self.asset_desc)) .then(|| Ok((asset, AssetSupply::new(value_sum, self.is_finalized())))) @@ -381,7 +339,7 @@ impl IssueBundle { // Make sure the `expected_ik` matches the `asset` for all notes. self.actions .iter() - .try_for_each(|action| action.compute_new_supply(&expected_ik).map(|_| ()))?; + .try_for_each(|action| action.verify_supply(&expected_ik).map(|_| ()))?; Ok(IssueBundle { ik: self.ik, @@ -461,9 +419,10 @@ pub fn verify_issue_bundle( sighash: [u8; 32], finalized: &mut HashSet, // The finalization set. ) -> Result { - if let Err(e) = bundle.ik.verify(&sighash, &bundle.authorization.signature) { - return Err(IssueBundleInvalidSignature(e)); - }; + bundle + .ik + .verify(&sighash, &bundle.authorization.signature) + .map_err(IssueBundleInvalidSignature)?; let supply_info = bundle @@ -474,18 +433,16 @@ pub fn verify_issue_bundle( return Err(WrongAssetDescSize); } - let (asset, supply) = action.compute_new_supply(bundle.ik())?; - - // Insert new supply into the supply info (fails if the asset was - // previously finalized or if the supply for the asset was already - // inserted and finalized) - if finalized.contains(&asset) - || (!supply_info.try_insert_asset_supply(asset, supply)) - { - Err(IssueActionPreviouslyFinalizedAssetBase(asset)) - } else { - Ok(supply_info) + let (asset, supply) = action.verify_supply(bundle.ik())?; + + // Fail if the asset was previously finalized. + if finalized.contains(&asset) { + return Err(IssueActionPreviouslyFinalizedAssetBase(asset)); } + + supply_info.add_supply(asset, supply)?; + + Ok(supply_info) })?; finalized.extend( @@ -607,7 +564,7 @@ mod tests { (rng, isk, ik, recipient, sighash) } - fn setup_compute_new_supply_test_params( + fn setup_verify_supply_test_params( asset_desc: &str, note1_value: u64, note2_value: u64, @@ -650,11 +607,11 @@ mod tests { } #[test] - fn test_compute_new_supply_valid() { + fn test_verify_supply_valid() { let (ik, test_asset, action) = - setup_compute_new_supply_test_params("Asset 1", 10, 20, None, false); + setup_verify_supply_test_params("Asset 1", 10, 20, None, false); - let result = action.compute_new_supply(&ik); + let result = action.verify_supply(&ik); assert!(result.is_ok()); @@ -666,11 +623,11 @@ mod tests { } #[test] - fn test_compute_new_supply_finalized() { + fn test_verify_supply_finalized() { let (ik, test_asset, action) = - setup_compute_new_supply_test_params("Asset 1", 10, 20, None, true); + setup_verify_supply_test_params("Asset 1", 10, 20, None, true); - let result = action.compute_new_supply(&ik); + let result = action.verify_supply(&ik); assert!(result.is_ok()); @@ -681,31 +638,24 @@ mod tests { assert!(supply.is_finalized); } - // FIXME: Evaluate the feasibility of implementing this test. Note values are - // represented by the NoteValue type (u64), while the supply amount uses the - // ValueSum type (i128). Due to the difference in types, simulating a supply - // amount overflow (i.e., the sum of values of notes) is challenging. - #[test] - fn test_compute_new_supply_value_sum_overflow() {} - #[test] - fn test_compute_new_supply_incorrect_asset_base() { + fn test_verify_supply_incorrect_asset_base() { let (ik, _, action) = - setup_compute_new_supply_test_params("Asset 1", 10, 20, Some("Asset 2"), false); + setup_verify_supply_test_params("Asset 1", 10, 20, Some("Asset 2"), false); assert_eq!( - action.compute_new_supply(&ik), + action.verify_supply(&ik), Err(IssueActionIncorrectAssetBase) ); } #[test] - fn test_compute_new_supply_ik_mismatch_asset_base() { - let (_, _, action) = setup_compute_new_supply_test_params("Asset 1", 10, 20, None, false); + fn test_verify_supply_ik_mismatch_asset_base() { + let (_, _, action) = setup_verify_supply_test_params("Asset 1", 10, 20, None, false); let (_, _, ik, _, _) = setup_params(); assert_eq!( - action.compute_new_supply(&ik), + action.verify_supply(&ik), Err(IssueBundleIkMismatchAssetBase) ); } diff --git a/src/lib.rs b/src/lib.rs index 76d9c41c0..22fa3fc22 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -25,6 +25,7 @@ mod constants; pub mod issuance; pub mod keys; pub mod note; +mod supply_info; // pub mod note_encryption; // disabled until backward compatability is implemented. pub mod note_encryption_v3; pub mod primitives; diff --git a/src/supply_info.rs b/src/supply_info.rs new file mode 100644 index 000000000..482e56968 --- /dev/null +++ b/src/supply_info.rs @@ -0,0 +1,99 @@ +//! Structs and logic related to supply information management for assets. + +use std::collections::{hash_map, HashMap}; + +use crate::{issuance::Error, note::AssetBase, value::ValueSum}; + +/// Represents the amount of an asset and its finalization status. +#[derive(Debug, Clone)] +#[cfg_attr(test, derive(PartialEq))] +pub struct AssetSupply { + /// The amount of the asset. + pub amount: ValueSum, + /// Whether or not the asset is finalized. + pub is_finalized: bool, +} + +impl AssetSupply { + /// Creates a new AssetSupply instance with the given amount and finalization status. + pub fn new(amount: ValueSum, is_finalized: bool) -> Self { + Self { + amount, + is_finalized, + } + } +} + +/// Contains information about the supply of assets. +#[derive(Debug, Clone, Default)] +pub struct SupplyInfo { + /// A map of asset bases to their respective supply information. + pub assets: HashMap, +} + +impl SupplyInfo { + /// Inserts or updates an asset's supply information in the supply info map. + /// If the asset exists, adds the amounts (unconditionally) and updates the finalization status + /// (only if the new supply is finalized). If the asset is not found, inserts the new supply. + pub fn add_supply(&mut self, asset: AssetBase, new_supply: AssetSupply) -> Result<(), Error> { + match self.assets.entry(asset) { + hash_map::Entry::Occupied(mut entry) => { + let supply = entry.get_mut(); + supply.amount = + (supply.amount + new_supply.amount).ok_or(Error::ValueSumOverflow)?; + supply.is_finalized |= new_supply.is_finalized; + } + hash_map::Entry::Vacant(entry) => { + entry.insert(new_supply); + } + } + + Ok(()) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::value::ValueSum; + + #[test] + fn test_add_supply_valid() { + let mut supply_info = SupplyInfo::default(); + + let asset = AssetBase::from_bytes(&[0u8; 32]).unwrap(); + + let supply1 = AssetSupply::new(ValueSum::from_raw(20), false); + let supply2 = AssetSupply::new(ValueSum::from_raw(30), true); + let supply3 = AssetSupply::new(ValueSum::from_raw(10), false); + let supply4 = AssetSupply::new(ValueSum::from_raw(10), true); + + assert!(supply_info.add_supply(asset, supply1).is_ok()); + + assert_eq!( + supply_info.assets.get(&asset), + Some(&AssetSupply::new(ValueSum::from_raw(20), false)) + ); + + assert!(supply_info.add_supply(asset, supply2).is_ok()); + + assert_eq!( + supply_info.assets.get(&asset), + Some(&AssetSupply::new(ValueSum::from_raw(50), true)) + ); + + assert!(supply_info.add_supply(asset, supply3).is_ok()); + + assert_eq!( + supply_info.assets.get(&asset), + Some(&AssetSupply::new(ValueSum::from_raw(60), true)) + ); + + assert!(supply_info.add_supply(asset, supply4).is_ok()); + + assert_eq!( + supply_info.assets.get(&asset), + Some(&AssetSupply::new(ValueSum::from_raw(70), true)) + ); + } +} From a9f5097d1e0cf9b078d41abb9a999cbfd3104cba Mon Sep 17 00:00:00 2001 From: Dmitry Demin Date: Tue, 25 Apr 2023 11:00:00 +0200 Subject: [PATCH 4/8] Partially resolve review remarks 3 (use into_mut for entry, SupplyInfo::new instead of default etc.) --- src/issuance.rs | 6 +++--- src/lib.rs | 2 +- src/supply_info.rs | 36 ++++++++++++++++++++++++++++-------- 3 files changed, 32 insertions(+), 12 deletions(-) diff --git a/src/issuance.rs b/src/issuance.rs index 50d6e5462..fb1528bb2 100644 --- a/src/issuance.rs +++ b/src/issuance.rs @@ -110,14 +110,14 @@ impl IssueAction { /// * `ValueSumOverflow`: If the total amount value of all notes in the `IssueAction` overflows. /// /// * `IssueBundleIkMismatchAssetBase`: If the provided `ik` is not used to derive the - /// `asset_id` for **all** internal notes. + /// asset base for **all** internal notes. fn verify_supply(&self, ik: &IssuanceValidatingKey) -> Result<(AssetBase, AssetSupply), Error> { // Calculate the value of the asset as a sum of values of all its notes // and ensure all note types are equal let (asset, value_sum) = self.notes.iter().try_fold( (self.notes().head.asset(), ValueSum::zero()), |(asset, value_sum), ¬e| { - // All assets should have the same asset + // All assets should have the same asset base note.asset() .eq(&asset) .then(|| ()) @@ -428,7 +428,7 @@ pub fn verify_issue_bundle( bundle .actions() .iter() - .try_fold(SupplyInfo::default(), |mut supply_info, action| { + .try_fold(SupplyInfo::new(), |mut supply_info, action| { if !is_asset_desc_of_valid_size(action.asset_desc()) { return Err(WrongAssetDescSize); } diff --git a/src/lib.rs b/src/lib.rs index 22fa3fc22..417116df2 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -25,7 +25,7 @@ mod constants; pub mod issuance; pub mod keys; pub mod note; -mod supply_info; +pub mod supply_info; // pub mod note_encryption; // disabled until backward compatability is implemented. pub mod note_encryption_v3; pub mod primitives; diff --git a/src/supply_info.rs b/src/supply_info.rs index 482e56968..5a7b7e2b7 100644 --- a/src/supply_info.rs +++ b/src/supply_info.rs @@ -25,20 +25,28 @@ impl AssetSupply { } /// Contains information about the supply of assets. -#[derive(Debug, Clone, Default)] +#[derive(Debug, Clone)] pub struct SupplyInfo { /// A map of asset bases to their respective supply information. pub assets: HashMap, } impl SupplyInfo { + /// Creates a new, empty `SupplyInfo` instance. + #[allow(clippy::new_without_default)] + pub fn new() -> Self { + Self { + assets: HashMap::new(), + } + } + /// Inserts or updates an asset's supply information in the supply info map. /// If the asset exists, adds the amounts (unconditionally) and updates the finalization status /// (only if the new supply is finalized). If the asset is not found, inserts the new supply. pub fn add_supply(&mut self, asset: AssetBase, new_supply: AssetSupply) -> Result<(), Error> { match self.assets.entry(asset) { - hash_map::Entry::Occupied(mut entry) => { - let supply = entry.get_mut(); + hash_map::Entry::Occupied(entry) => { + let supply = entry.into_mut(); supply.amount = (supply.amount + new_supply.amount).ok_or(Error::ValueSumOverflow)?; supply.is_finalized |= new_supply.is_finalized; @@ -52,6 +60,12 @@ impl SupplyInfo { } } +impl Default for SupplyInfo { + fn default() -> Self { + Self::new() + } +} + #[cfg(test)] mod tests { use super::*; @@ -59,7 +73,7 @@ mod tests { #[test] fn test_add_supply_valid() { - let mut supply_info = SupplyInfo::default(); + let mut supply_info = SupplyInfo::new(); let asset = AssetBase::from_bytes(&[0u8; 32]).unwrap(); @@ -68,29 +82,35 @@ mod tests { let supply3 = AssetSupply::new(ValueSum::from_raw(10), false); let supply4 = AssetSupply::new(ValueSum::from_raw(10), true); - assert!(supply_info.add_supply(asset, supply1).is_ok()); + assert_eq!(supply_info.assets.len(), 0); + // Add supply1 + assert!(supply_info.add_supply(asset, supply1).is_ok()); + assert_eq!(supply_info.assets.len(), 1); assert_eq!( supply_info.assets.get(&asset), Some(&AssetSupply::new(ValueSum::from_raw(20), false)) ); + // Add supply2 assert!(supply_info.add_supply(asset, supply2).is_ok()); - + assert_eq!(supply_info.assets.len(), 1); assert_eq!( supply_info.assets.get(&asset), Some(&AssetSupply::new(ValueSum::from_raw(50), true)) ); + // Add supply3 assert!(supply_info.add_supply(asset, supply3).is_ok()); - + assert_eq!(supply_info.assets.len(), 1); assert_eq!( supply_info.assets.get(&asset), Some(&AssetSupply::new(ValueSum::from_raw(60), true)) ); + // Add supply4 assert!(supply_info.add_supply(asset, supply4).is_ok()); - + assert_eq!(supply_info.assets.len(), 1); assert_eq!( supply_info.assets.get(&asset), Some(&AssetSupply::new(ValueSum::from_raw(70), true)) From 9a5c85395eef12112f6b07ff4666f266a3ee5861 Mon Sep 17 00:00:00 2001 From: Dmitry Demin Date: Tue, 25 Apr 2023 15:35:36 +0200 Subject: [PATCH 5/8] Add update_finalized_assets method to SupplyInfo and use after the calls of verify_issue_bundle function (if needed), instead of mutating the finalization set inside verify_issue_bundle --- src/issuance.rs | 80 +++++++++++++++++++---------------------- src/supply_info.rs | 88 ++++++++++++++++++++++++++++++++++++++++------ tests/zsa.rs | 2 +- 3 files changed, 114 insertions(+), 56 deletions(-) diff --git a/src/issuance.rs b/src/issuance.rs index fb1528bb2..24fdc976f 100644 --- a/src/issuance.rs +++ b/src/issuance.rs @@ -381,8 +381,7 @@ impl IssueBundle { /// Validation for Orchard IssueBundles /// -/// A set of previously finalized asset types must be provided. -/// In case of success, `finalized` will contain a set of the provided **and** the newly finalized `AssetBase`s +/// A set of previously finalized asset types must be provided in `finalized` argument. /// /// The following checks are performed: /// * For the `IssueBundle`: @@ -417,7 +416,7 @@ impl IssueBundle { pub fn verify_issue_bundle( bundle: &IssueBundle, sighash: [u8; 32], - finalized: &mut HashSet, // The finalization set. + finalized: &HashSet, // The finalization set. ) -> Result { bundle .ik @@ -445,13 +444,6 @@ pub fn verify_issue_bundle( Ok(supply_info) })?; - finalized.extend( - supply_info - .assets - .iter() - .filter_map(|(asset, supply)| supply.is_finalized.then(|| asset)), - ); - Ok(supply_info) } @@ -930,11 +922,12 @@ mod tests { .unwrap(); let signed = bundle.prepare(sighash).sign(rng, &isk).unwrap(); - let prev_finalized = &mut HashSet::new(); - let res = verify_issue_bundle(&signed, sighash, prev_finalized); - assert!(res.is_ok()); + let supply_info = verify_issue_bundle(&signed, sighash, prev_finalized).unwrap(); + + supply_info.update_finalized_assets(prev_finalized); + assert!(prev_finalized.is_empty()); } @@ -955,16 +948,14 @@ mod tests { .unwrap(); let signed = bundle.prepare(sighash).sign(rng, &isk).unwrap(); - let prev_finalized = &mut HashSet::new(); - let res = verify_issue_bundle(&signed, sighash, prev_finalized); - assert!(res.is_ok()); - assert!(prev_finalized.contains(&AssetBase::derive( - &ik, - &String::from("Verify with finalize") - ))); + let supply_info = verify_issue_bundle(&signed, sighash, prev_finalized).unwrap(); + + supply_info.update_finalized_assets(prev_finalized); + assert_eq!(prev_finalized.len(), 1); + assert!(prev_finalized.contains(&AssetBase::derive(&ik, "Verify with finalize"))); } #[test] @@ -1022,12 +1013,11 @@ mod tests { .unwrap(); let signed = bundle.prepare(sighash).sign(rng, &isk).unwrap(); - let prev_finalized = &mut HashSet::new(); - let res = verify_issue_bundle(&signed, sighash, prev_finalized); + let supply_info = verify_issue_bundle(&signed, sighash, prev_finalized).unwrap(); - assert!(res.is_ok()); + supply_info.update_finalized_assets(prev_finalized); assert_eq!(prev_finalized.len(), 2); @@ -1035,8 +1025,6 @@ mod tests { assert!(prev_finalized.contains(&asset2_base)); assert!(!prev_finalized.contains(&asset3_base)); - let supply_info = res.unwrap(); - assert_eq!(supply_info.assets.len(), 3); assert_eq!( @@ -1076,9 +1064,8 @@ mod tests { prev_finalized.insert(final_type); - let finalized = verify_issue_bundle(&signed, sighash, prev_finalized); assert_eq!( - finalized.unwrap_err(), + verify_issue_bundle(&signed, sighash, prev_finalized).unwrap_err(), IssueActionPreviouslyFinalizedAssetBase(final_type) ); } @@ -1114,7 +1101,7 @@ mod tests { signature: wrong_isk.sign(&mut rng, &sighash), }); - let prev_finalized = &mut HashSet::new(); + let prev_finalized = &HashSet::new(); assert_eq!( verify_issue_bundle(&signed, sighash, prev_finalized).unwrap_err(), @@ -1139,13 +1126,10 @@ mod tests { let sighash: [u8; 32] = bundle.commitment().into(); let signed = bundle.prepare(sighash).sign(rng, &isk).unwrap(); - let prev_finalized = &mut HashSet::new(); - - // 2. Try empty description - let finalized = verify_issue_bundle(&signed, random_sighash, prev_finalized); + let prev_finalized = &HashSet::new(); assert_eq!( - finalized.unwrap_err(), + verify_issue_bundle(&signed, random_sighash, prev_finalized).unwrap_err(), IssueBundleInvalidSignature(InvalidSignature) ); } @@ -1185,10 +1169,12 @@ mod tests { .borrow_mut() .push(note); - let prev_finalized = &mut HashSet::new(); - let err = verify_issue_bundle(&signed, sighash, prev_finalized).unwrap_err(); + let prev_finalized = &HashSet::new(); - assert_eq!(err, IssueActionIncorrectAssetBase); + assert_eq!( + verify_issue_bundle(&signed, sighash, prev_finalized).unwrap_err(), + IssueActionIncorrectAssetBase + ); } #[test] @@ -1226,10 +1212,12 @@ mod tests { signed.actions.first_mut().unwrap().notes = NonEmpty::new(note); - let prev_finalized = &mut HashSet::new(); - let err = verify_issue_bundle(&signed, sighash, prev_finalized).unwrap_err(); + let prev_finalized = &HashSet::new(); - assert_eq!(err, IssueBundleIkMismatchAssetBase); + assert_eq!( + verify_issue_bundle(&signed, sighash, prev_finalized).unwrap_err(), + IssueBundleIkMismatchAssetBase + ); } #[test] @@ -1256,7 +1244,7 @@ mod tests { .unwrap(); let mut signed = bundle.prepare(sighash).sign(rng, &isk).unwrap(); - let prev_finalized = &mut HashSet::new(); + let prev_finalized = HashSet::new(); // 1. Try too long description signed @@ -1264,9 +1252,11 @@ mod tests { .first_mut() .unwrap() .modify_descr(String::from_utf8(vec![b'X'; 513]).unwrap()); - let finalized = verify_issue_bundle(&signed, sighash, prev_finalized); - assert_eq!(finalized.unwrap_err(), WrongAssetDescSize); + assert_eq!( + verify_issue_bundle(&signed, sighash, &prev_finalized).unwrap_err(), + WrongAssetDescSize + ); // 2. Try empty description signed @@ -1274,9 +1264,11 @@ mod tests { .first_mut() .unwrap() .modify_descr("".to_string()); - let finalized = verify_issue_bundle(&signed, sighash, prev_finalized); - assert_eq!(finalized.unwrap_err(), WrongAssetDescSize); + assert_eq!( + verify_issue_bundle(&signed, sighash, &prev_finalized).unwrap_err(), + WrongAssetDescSize + ); } } diff --git a/src/supply_info.rs b/src/supply_info.rs index 5a7b7e2b7..ba2a9d300 100644 --- a/src/supply_info.rs +++ b/src/supply_info.rs @@ -1,6 +1,6 @@ //! Structs and logic related to supply information management for assets. -use std::collections::{hash_map, HashMap}; +use std::collections::{hash_map, HashMap, HashSet}; use crate::{issuance::Error, note::AssetBase, value::ValueSum}; @@ -58,6 +58,16 @@ impl SupplyInfo { Ok(()) } + + /// Updates the set of finalized assets based on the supply information stored in + /// the `SupplyInfo` instance. + pub fn update_finalized_assets(&self, finalized_assets: &mut HashSet) { + finalized_assets.extend( + self.assets + .iter() + .filter_map(|(asset, supply)| supply.is_finalized.then(|| asset)), + ); + } } impl Default for SupplyInfo { @@ -69,51 +79,107 @@ impl Default for SupplyInfo { #[cfg(test)] mod tests { use super::*; - use crate::value::ValueSum; + + fn create_random_asset(seed: u64) -> AssetBase { + use { + group::{Group, GroupEncoding}, + pasta_curves::pallas::Point, + rand::{rngs::StdRng, SeedableRng}, + }; + + AssetBase::from_bytes(&Point::random(StdRng::seed_from_u64(seed)).to_bytes()).unwrap() + } #[test] fn test_add_supply_valid() { let mut supply_info = SupplyInfo::new(); - let asset = AssetBase::from_bytes(&[0u8; 32]).unwrap(); + let asset1 = create_random_asset(1); + let asset2 = create_random_asset(2); let supply1 = AssetSupply::new(ValueSum::from_raw(20), false); let supply2 = AssetSupply::new(ValueSum::from_raw(30), true); let supply3 = AssetSupply::new(ValueSum::from_raw(10), false); let supply4 = AssetSupply::new(ValueSum::from_raw(10), true); + let supply5 = AssetSupply::new(ValueSum::from_raw(50), false); assert_eq!(supply_info.assets.len(), 0); // Add supply1 - assert!(supply_info.add_supply(asset, supply1).is_ok()); + assert!(supply_info.add_supply(asset1, supply1).is_ok()); assert_eq!(supply_info.assets.len(), 1); assert_eq!( - supply_info.assets.get(&asset), + supply_info.assets.get(&asset1), Some(&AssetSupply::new(ValueSum::from_raw(20), false)) ); // Add supply2 - assert!(supply_info.add_supply(asset, supply2).is_ok()); + assert!(supply_info.add_supply(asset1, supply2).is_ok()); assert_eq!(supply_info.assets.len(), 1); assert_eq!( - supply_info.assets.get(&asset), + supply_info.assets.get(&asset1), Some(&AssetSupply::new(ValueSum::from_raw(50), true)) ); // Add supply3 - assert!(supply_info.add_supply(asset, supply3).is_ok()); + assert!(supply_info.add_supply(asset1, supply3).is_ok()); assert_eq!(supply_info.assets.len(), 1); assert_eq!( - supply_info.assets.get(&asset), + supply_info.assets.get(&asset1), Some(&AssetSupply::new(ValueSum::from_raw(60), true)) ); // Add supply4 - assert!(supply_info.add_supply(asset, supply4).is_ok()); + assert!(supply_info.add_supply(asset1, supply4).is_ok()); assert_eq!(supply_info.assets.len(), 1); assert_eq!( - supply_info.assets.get(&asset), + supply_info.assets.get(&asset1), Some(&AssetSupply::new(ValueSum::from_raw(70), true)) ); + + // Add supply4 + assert!(supply_info.add_supply(asset2, supply5).is_ok()); + assert_eq!(supply_info.assets.len(), 2); + assert_eq!( + supply_info.assets.get(&asset1), + Some(&AssetSupply::new(ValueSum::from_raw(70), true)) + ); + assert_eq!( + supply_info.assets.get(&asset2), + Some(&AssetSupply::new(ValueSum::from_raw(50), false)) + ); + } + + #[test] + fn test_update_finalized_assets() { + let mut supply_info = SupplyInfo::new(); + + let asset1 = create_random_asset(1); + let asset2 = create_random_asset(2); + let asset3 = create_random_asset(3); + + assert!(supply_info + .add_supply(asset1, AssetSupply::new(ValueSum::from_raw(10), false)) + .is_ok()); + assert!(supply_info + .add_supply(asset1, AssetSupply::new(ValueSum::from_raw(20), true)) + .is_ok()); + + assert!(supply_info + .add_supply(asset2, AssetSupply::new(ValueSum::from_raw(40), false)) + .is_ok()); + + assert!(supply_info + .add_supply(asset3, AssetSupply::new(ValueSum::from_raw(50), true)) + .is_ok()); + + let mut finalized_assets = HashSet::new(); + + supply_info.update_finalized_assets(&mut finalized_assets); + + assert_eq!(finalized_assets.len(), 2); + + assert!(finalized_assets.contains(&asset1)); + assert!(finalized_assets.contains(&asset3)); } } diff --git a/tests/zsa.rs b/tests/zsa.rs index 5b1ea4437..e597ff5c5 100644 --- a/tests/zsa.rs +++ b/tests/zsa.rs @@ -169,7 +169,7 @@ fn issue_zsa_notes(asset_descr: &str, keys: &Keychain) -> (Note, Note) { assert!(verify_issue_bundle( &issue_bundle, issue_bundle.commitment().into(), - &mut HashSet::new(), + &HashSet::new(), ) .is_ok()); From 9659d3c2d1cc369a04566727d66fa7bba6af8065 Mon Sep 17 00:00:00 2001 From: Dmitry Demin Date: Tue, 2 May 2023 16:23:14 +0200 Subject: [PATCH 6/8] Resolve review 4 remarks (compute supply sum from vars and use AssetBase::derive in tests etc.) --- src/issuance.rs | 73 ++++++++++++++++--------------- src/supply_info.rs | 106 +++++++++++++++++++++++++-------------------- 2 files changed, 97 insertions(+), 82 deletions(-) diff --git a/src/issuance.rs b/src/issuance.rs index 24fdc976f..26377b3cc 100644 --- a/src/issuance.rs +++ b/src/issuance.rs @@ -110,14 +110,14 @@ impl IssueAction { /// * `ValueSumOverflow`: If the total amount value of all notes in the `IssueAction` overflows. /// /// * `IssueBundleIkMismatchAssetBase`: If the provided `ik` is not used to derive the - /// asset base for **all** internal notes. + /// `AssetBase` for **all** internal notes. fn verify_supply(&self, ik: &IssuanceValidatingKey) -> Result<(AssetBase, AssetSupply), Error> { // Calculate the value of the asset as a sum of values of all its notes // and ensure all note types are equal let (asset, value_sum) = self.notes.iter().try_fold( (self.notes().head.asset(), ValueSum::zero()), |(asset, value_sum), ¬e| { - // All assets should have the same asset base + // All assets should have the same `AssetBase` note.asset() .eq(&asset) .then(|| ()) @@ -337,9 +337,10 @@ impl IssueBundle { let expected_ik: IssuanceValidatingKey = (isk).into(); // Make sure the `expected_ik` matches the `asset` for all notes. - self.actions - .iter() - .try_for_each(|action| action.verify_supply(&expected_ik).map(|_| ()))?; + self.actions.iter().try_for_each(|action| { + action.verify_supply(&expected_ik)?; + Ok(()) + })?; Ok(IssueBundle { ik: self.ik, @@ -535,37 +536,16 @@ mod tests { use std::borrow::BorrowMut; use std::collections::HashSet; - fn setup_params() -> ( - OsRng, - IssuanceAuthorizingKey, - IssuanceValidatingKey, - Address, - [u8; 32], - ) { - let mut rng = OsRng; - let sk = SpendingKey::random(&mut rng); - let isk: IssuanceAuthorizingKey = (&sk).into(); - let ik: IssuanceValidatingKey = (&isk).into(); - - let fvk = FullViewingKey::from(&sk); - let recipient = fvk.address_at(0u32, Scope::External); - - let mut sighash = [0u8; 32]; - rng.fill_bytes(&mut sighash); - - (rng, isk, ik, recipient, sighash) - } - fn setup_verify_supply_test_params( - asset_desc: &str, note1_value: u64, note2_value: u64, + note1_asset_desc: &str, note2_asset_desc: Option<&str>, // if None, both notes use the same asset finalize: bool, ) -> (IssuanceValidatingKey, AssetBase, IssueAction) { let (mut rng, _, ik, recipient, _) = setup_params(); - let asset = AssetBase::derive(&ik, asset_desc); + let asset = AssetBase::derive(&ik, note1_asset_desc); let note2_asset = note2_asset_desc.map_or(asset, |desc| AssetBase::derive(&ik, desc)); let note1 = Note::new( @@ -588,7 +568,7 @@ mod tests { ik, asset, IssueAction::from_parts( - asset_desc.into(), + note1_asset_desc.into(), NonEmpty { head: note1, tail: vec![note2], @@ -601,7 +581,7 @@ mod tests { #[test] fn test_verify_supply_valid() { let (ik, test_asset, action) = - setup_verify_supply_test_params("Asset 1", 10, 20, None, false); + setup_verify_supply_test_params(10, 20, "Asset 1", None, false); let result = action.verify_supply(&ik); @@ -617,7 +597,7 @@ mod tests { #[test] fn test_verify_supply_finalized() { let (ik, test_asset, action) = - setup_verify_supply_test_params("Asset 1", 10, 20, None, true); + setup_verify_supply_test_params(10, 20, "Asset 1", None, true); let result = action.verify_supply(&ik); @@ -633,7 +613,7 @@ mod tests { #[test] fn test_verify_supply_incorrect_asset_base() { let (ik, _, action) = - setup_verify_supply_test_params("Asset 1", 10, 20, Some("Asset 2"), false); + setup_verify_supply_test_params(10, 20, "Asset 1", Some("Asset 2"), false); assert_eq!( action.verify_supply(&ik), @@ -643,7 +623,7 @@ mod tests { #[test] fn test_verify_supply_ik_mismatch_asset_base() { - let (_, _, action) = setup_verify_supply_test_params("Asset 1", 10, 20, None, false); + let (_, _, action) = setup_verify_supply_test_params(10, 20, "Asset 1", None, false); let (_, _, ik, _, _) = setup_params(); assert_eq!( @@ -652,6 +632,27 @@ mod tests { ); } + fn setup_params() -> ( + OsRng, + IssuanceAuthorizingKey, + IssuanceValidatingKey, + Address, + [u8; 32], + ) { + let mut rng = OsRng; + let sk = SpendingKey::random(&mut rng); + let isk: IssuanceAuthorizingKey = (&sk).into(); + let ik: IssuanceValidatingKey = (&isk).into(); + + let fvk = FullViewingKey::from(&sk); + let recipient = fvk.address_at(0u32, Scope::External); + + let mut sighash = [0u8; 32]; + rng.fill_bytes(&mut sighash); + + (rng, isk, ik, recipient, sighash) + } + #[test] fn issue_bundle_basic() { let (rng, _, ik, recipient, _) = setup_params(); @@ -926,7 +927,7 @@ mod tests { let supply_info = verify_issue_bundle(&signed, sighash, prev_finalized).unwrap(); - supply_info.update_finalized_assets(prev_finalized); + supply_info.update_finalization_set(prev_finalized); assert!(prev_finalized.is_empty()); } @@ -952,7 +953,7 @@ mod tests { let supply_info = verify_issue_bundle(&signed, sighash, prev_finalized).unwrap(); - supply_info.update_finalized_assets(prev_finalized); + supply_info.update_finalization_set(prev_finalized); assert_eq!(prev_finalized.len(), 1); assert!(prev_finalized.contains(&AssetBase::derive(&ik, "Verify with finalize"))); @@ -1017,7 +1018,7 @@ mod tests { let supply_info = verify_issue_bundle(&signed, sighash, prev_finalized).unwrap(); - supply_info.update_finalized_assets(prev_finalized); + supply_info.update_finalization_set(prev_finalized); assert_eq!(prev_finalized.len(), 2); diff --git a/src/supply_info.rs b/src/supply_info.rs index ba2a9d300..a1967d13d 100644 --- a/src/supply_info.rs +++ b/src/supply_info.rs @@ -33,7 +33,6 @@ pub struct SupplyInfo { impl SupplyInfo { /// Creates a new, empty `SupplyInfo` instance. - #[allow(clippy::new_without_default)] pub fn new() -> Self { Self { assets: HashMap::new(), @@ -61,8 +60,8 @@ impl SupplyInfo { /// Updates the set of finalized assets based on the supply information stored in /// the `SupplyInfo` instance. - pub fn update_finalized_assets(&self, finalized_assets: &mut HashSet) { - finalized_assets.extend( + pub fn update_finalization_set(&self, finalization_set: &mut HashSet) { + finalization_set.extend( self.assets .iter() .filter_map(|(asset, supply)| supply.is_finalized.then(|| asset)), @@ -80,22 +79,30 @@ impl Default for SupplyInfo { mod tests { use super::*; - fn create_random_asset(seed: u64) -> AssetBase { - use { - group::{Group, GroupEncoding}, - pasta_curves::pallas::Point, - rand::{rngs::StdRng, SeedableRng}, - }; + fn create_random_asset(asset_desc: &str) -> AssetBase { + use crate::keys::{IssuanceAuthorizingKey, IssuanceValidatingKey, SpendingKey}; - AssetBase::from_bytes(&Point::random(StdRng::seed_from_u64(seed)).to_bytes()).unwrap() + let sk = SpendingKey::from_bytes([0u8; 32]).unwrap(); + let isk: IssuanceAuthorizingKey = (&sk).into(); + + AssetBase::derive(&IssuanceValidatingKey::from(&isk), asset_desc) + } + + fn sum_amounts<'a, Supplies: IntoIterator>( + supplies: Supplies, + ) -> Option { + supplies + .into_iter() + .map(|supply| supply.amount) + .try_fold(ValueSum::from_raw(0), |sum, value| sum + value) } #[test] fn test_add_supply_valid() { let mut supply_info = SupplyInfo::new(); - let asset1 = create_random_asset(1); - let asset2 = create_random_asset(2); + let asset1 = create_random_asset("Asset 1"); + let asset2 = create_random_asset("Asset 2"); let supply1 = AssetSupply::new(ValueSum::from_raw(20), false); let supply2 = AssetSupply::new(ValueSum::from_raw(30), true); @@ -106,80 +113,87 @@ mod tests { assert_eq!(supply_info.assets.len(), 0); // Add supply1 - assert!(supply_info.add_supply(asset1, supply1).is_ok()); + assert!(supply_info.add_supply(asset1, supply1.clone()).is_ok()); assert_eq!(supply_info.assets.len(), 1); assert_eq!( supply_info.assets.get(&asset1), - Some(&AssetSupply::new(ValueSum::from_raw(20), false)) + Some(&AssetSupply::new(sum_amounts([&supply1]).unwrap(), false)) ); // Add supply2 - assert!(supply_info.add_supply(asset1, supply2).is_ok()); + assert!(supply_info.add_supply(asset1, supply2.clone()).is_ok()); assert_eq!(supply_info.assets.len(), 1); assert_eq!( supply_info.assets.get(&asset1), - Some(&AssetSupply::new(ValueSum::from_raw(50), true)) + Some(&AssetSupply::new( + sum_amounts([&supply1, &supply2]).unwrap(), + true + )) ); // Add supply3 - assert!(supply_info.add_supply(asset1, supply3).is_ok()); + assert!(supply_info.add_supply(asset1, supply3.clone()).is_ok()); assert_eq!(supply_info.assets.len(), 1); assert_eq!( supply_info.assets.get(&asset1), - Some(&AssetSupply::new(ValueSum::from_raw(60), true)) + Some(&AssetSupply::new( + sum_amounts([&supply1, &supply2, &supply3]).unwrap(), + true + )) ); // Add supply4 - assert!(supply_info.add_supply(asset1, supply4).is_ok()); + assert!(supply_info.add_supply(asset1, supply4.clone()).is_ok()); assert_eq!(supply_info.assets.len(), 1); assert_eq!( supply_info.assets.get(&asset1), - Some(&AssetSupply::new(ValueSum::from_raw(70), true)) + Some(&AssetSupply::new( + sum_amounts([&supply1, &supply2, &supply3, &supply4]).unwrap(), + true + )) ); - // Add supply4 - assert!(supply_info.add_supply(asset2, supply5).is_ok()); + // Add supply5 + assert!(supply_info.add_supply(asset2, supply5.clone()).is_ok()); assert_eq!(supply_info.assets.len(), 2); assert_eq!( supply_info.assets.get(&asset1), - Some(&AssetSupply::new(ValueSum::from_raw(70), true)) + Some(&AssetSupply::new( + sum_amounts([&supply1, &supply2, &supply3, &supply4]).unwrap(), + true + )) ); assert_eq!( supply_info.assets.get(&asset2), - Some(&AssetSupply::new(ValueSum::from_raw(50), false)) + Some(&AssetSupply::new(sum_amounts([&supply5]).unwrap(), false)) ); } #[test] - fn test_update_finalized_assets() { + fn test_update_finalization_set() { let mut supply_info = SupplyInfo::new(); - let asset1 = create_random_asset(1); - let asset2 = create_random_asset(2); - let asset3 = create_random_asset(3); - - assert!(supply_info - .add_supply(asset1, AssetSupply::new(ValueSum::from_raw(10), false)) - .is_ok()); - assert!(supply_info - .add_supply(asset1, AssetSupply::new(ValueSum::from_raw(20), true)) - .is_ok()); + let asset1 = create_random_asset("Asset 1"); + let asset2 = create_random_asset("Asset 2"); + let asset3 = create_random_asset("Asset 3"); - assert!(supply_info - .add_supply(asset2, AssetSupply::new(ValueSum::from_raw(40), false)) - .is_ok()); + let supply1 = AssetSupply::new(ValueSum::from_raw(10), false); + let supply2 = AssetSupply::new(ValueSum::from_raw(20), true); + let supply3 = AssetSupply::new(ValueSum::from_raw(40), false); + let supply4 = AssetSupply::new(ValueSum::from_raw(50), true); - assert!(supply_info - .add_supply(asset3, AssetSupply::new(ValueSum::from_raw(50), true)) - .is_ok()); + assert!(supply_info.add_supply(asset1, supply1).is_ok()); + assert!(supply_info.add_supply(asset1, supply2).is_ok()); + assert!(supply_info.add_supply(asset2, supply3).is_ok()); + assert!(supply_info.add_supply(asset3, supply4).is_ok()); - let mut finalized_assets = HashSet::new(); + let mut finalization_set = HashSet::new(); - supply_info.update_finalized_assets(&mut finalized_assets); + supply_info.update_finalization_set(&mut finalization_set); - assert_eq!(finalized_assets.len(), 2); + assert_eq!(finalization_set.len(), 2); - assert!(finalized_assets.contains(&asset1)); - assert!(finalized_assets.contains(&asset3)); + assert!(finalization_set.contains(&asset1)); + assert!(finalization_set.contains(&asset3)); } } From ad32b7b05b2f0310137fb3d9e75e8285a5c9ba8e Mon Sep 17 00:00:00 2001 From: Dmitry Demin Date: Tue, 2 May 2023 21:18:39 +0200 Subject: [PATCH 7/8] Rename create_random_asset to create_test_asset in supply_info --- src/supply_info.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/supply_info.rs b/src/supply_info.rs index a1967d13d..dc02507d9 100644 --- a/src/supply_info.rs +++ b/src/supply_info.rs @@ -79,7 +79,7 @@ impl Default for SupplyInfo { mod tests { use super::*; - fn create_random_asset(asset_desc: &str) -> AssetBase { + fn create_test_asset(asset_desc: &str) -> AssetBase { use crate::keys::{IssuanceAuthorizingKey, IssuanceValidatingKey, SpendingKey}; let sk = SpendingKey::from_bytes([0u8; 32]).unwrap(); @@ -101,8 +101,8 @@ mod tests { fn test_add_supply_valid() { let mut supply_info = SupplyInfo::new(); - let asset1 = create_random_asset("Asset 1"); - let asset2 = create_random_asset("Asset 2"); + let asset1 = create_test_asset("Asset 1"); + let asset2 = create_test_asset("Asset 2"); let supply1 = AssetSupply::new(ValueSum::from_raw(20), false); let supply2 = AssetSupply::new(ValueSum::from_raw(30), true); @@ -173,9 +173,9 @@ mod tests { fn test_update_finalization_set() { let mut supply_info = SupplyInfo::new(); - let asset1 = create_random_asset("Asset 1"); - let asset2 = create_random_asset("Asset 2"); - let asset3 = create_random_asset("Asset 3"); + let asset1 = create_test_asset("Asset 1"); + let asset2 = create_test_asset("Asset 2"); + let asset3 = create_test_asset("Asset 3"); let supply1 = AssetSupply::new(ValueSum::from_raw(10), false); let supply2 = AssetSupply::new(ValueSum::from_raw(20), true); From 86075812fb481677aa3751e5b487d4abebfd4445 Mon Sep 17 00:00:00 2001 From: Dmitry Demin Date: Thu, 4 May 2023 10:59:44 +0200 Subject: [PATCH 8/8] Resolve review 5 remarks (rename sum_amounts to sum, add Copy for AssetSupply, fix setup_params) --- src/issuance.rs | 43 ++++++++++++++++++++++--------------------- src/supply_info.rs | 31 +++++++++++++------------------ 2 files changed, 35 insertions(+), 39 deletions(-) diff --git a/src/issuance.rs b/src/issuance.rs index 26377b3cc..af243358e 100644 --- a/src/issuance.rs +++ b/src/issuance.rs @@ -536,6 +536,28 @@ mod tests { use std::borrow::BorrowMut; use std::collections::HashSet; + fn setup_params() -> ( + OsRng, + IssuanceAuthorizingKey, + IssuanceValidatingKey, + Address, + [u8; 32], + ) { + let mut rng = OsRng; + + let sk = SpendingKey::random(&mut rng); + let isk: IssuanceAuthorizingKey = (&sk).into(); + let ik: IssuanceValidatingKey = (&isk).into(); + + let fvk = FullViewingKey::from(&SpendingKey::random(&mut rng)); + let recipient = fvk.address_at(0u32, Scope::External); + + let mut sighash = [0u8; 32]; + rng.fill_bytes(&mut sighash); + + (rng, isk, ik, recipient, sighash) + } + fn setup_verify_supply_test_params( note1_value: u64, note2_value: u64, @@ -632,27 +654,6 @@ mod tests { ); } - fn setup_params() -> ( - OsRng, - IssuanceAuthorizingKey, - IssuanceValidatingKey, - Address, - [u8; 32], - ) { - let mut rng = OsRng; - let sk = SpendingKey::random(&mut rng); - let isk: IssuanceAuthorizingKey = (&sk).into(); - let ik: IssuanceValidatingKey = (&isk).into(); - - let fvk = FullViewingKey::from(&sk); - let recipient = fvk.address_at(0u32, Scope::External); - - let mut sighash = [0u8; 32]; - rng.fill_bytes(&mut sighash); - - (rng, isk, ik, recipient, sighash) - } - #[test] fn issue_bundle_basic() { let (rng, _, ik, recipient, _) = setup_params(); diff --git a/src/supply_info.rs b/src/supply_info.rs index dc02507d9..6bd96df34 100644 --- a/src/supply_info.rs +++ b/src/supply_info.rs @@ -5,7 +5,7 @@ use std::collections::{hash_map, HashMap, HashSet}; use crate::{issuance::Error, note::AssetBase, value::ValueSum}; /// Represents the amount of an asset and its finalization status. -#[derive(Debug, Clone)] +#[derive(Debug, Clone, Copy)] #[cfg_attr(test, derive(PartialEq))] pub struct AssetSupply { /// The amount of the asset. @@ -88,9 +88,7 @@ mod tests { AssetBase::derive(&IssuanceValidatingKey::from(&isk), asset_desc) } - fn sum_amounts<'a, Supplies: IntoIterator>( - supplies: Supplies, - ) -> Option { + fn sum<'a, T: IntoIterator>(supplies: T) -> Option { supplies .into_iter() .map(|supply| supply.amount) @@ -113,59 +111,56 @@ mod tests { assert_eq!(supply_info.assets.len(), 0); // Add supply1 - assert!(supply_info.add_supply(asset1, supply1.clone()).is_ok()); + assert!(supply_info.add_supply(asset1, supply1).is_ok()); assert_eq!(supply_info.assets.len(), 1); assert_eq!( supply_info.assets.get(&asset1), - Some(&AssetSupply::new(sum_amounts([&supply1]).unwrap(), false)) + Some(&AssetSupply::new(sum([&supply1]).unwrap(), false)) ); // Add supply2 - assert!(supply_info.add_supply(asset1, supply2.clone()).is_ok()); + assert!(supply_info.add_supply(asset1, supply2).is_ok()); assert_eq!(supply_info.assets.len(), 1); assert_eq!( supply_info.assets.get(&asset1), - Some(&AssetSupply::new( - sum_amounts([&supply1, &supply2]).unwrap(), - true - )) + Some(&AssetSupply::new(sum([&supply1, &supply2]).unwrap(), true)) ); // Add supply3 - assert!(supply_info.add_supply(asset1, supply3.clone()).is_ok()); + assert!(supply_info.add_supply(asset1, supply3).is_ok()); assert_eq!(supply_info.assets.len(), 1); assert_eq!( supply_info.assets.get(&asset1), Some(&AssetSupply::new( - sum_amounts([&supply1, &supply2, &supply3]).unwrap(), + sum([&supply1, &supply2, &supply3]).unwrap(), true )) ); // Add supply4 - assert!(supply_info.add_supply(asset1, supply4.clone()).is_ok()); + assert!(supply_info.add_supply(asset1, supply4).is_ok()); assert_eq!(supply_info.assets.len(), 1); assert_eq!( supply_info.assets.get(&asset1), Some(&AssetSupply::new( - sum_amounts([&supply1, &supply2, &supply3, &supply4]).unwrap(), + sum([&supply1, &supply2, &supply3, &supply4]).unwrap(), true )) ); // Add supply5 - assert!(supply_info.add_supply(asset2, supply5.clone()).is_ok()); + assert!(supply_info.add_supply(asset2, supply5).is_ok()); assert_eq!(supply_info.assets.len(), 2); assert_eq!( supply_info.assets.get(&asset1), Some(&AssetSupply::new( - sum_amounts([&supply1, &supply2, &supply3, &supply4]).unwrap(), + sum([&supply1, &supply2, &supply3, &supply4]).unwrap(), true )) ); assert_eq!( supply_info.assets.get(&asset2), - Some(&AssetSupply::new(sum_amounts([&supply5]).unwrap(), false)) + Some(&AssetSupply::new(sum([&supply5]).unwrap(), false)) ); }