Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tracking for supply info inside verify_issue_bundle #55

Merged
merged 8 commits into from
May 4, 2023
Merged

Add tracking for supply info inside verify_issue_bundle #55

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
fc2f811
Add tracking for supply info inside verify_issue_bundle
dmidem Apr 19, 2023
dc84dbf
Resolve review remarks (fix issues, add docs, tests for compute_new_s…
dmidem Apr 22, 2023
10e7aa0
Resolve review remarks 2 (rename compute_new_supply to verify_supply,…
dmidem Apr 24, 2023
a9f5097
Partially resolve review remarks 3 (use into_mut for entry, SupplyInf…
dmidem Apr 25, 2023
9a5c853
Add update_finalized_assets method to SupplyInfo and use after the ca…
dmidem Apr 25, 2023
9659d3c
Resolve review 4 remarks (compute supply sum from vars and use AssetB…
dmidem May 2, 2023
ad32b7b
Rename create_random_asset to create_test_asset in supply_info
dmidem May 2, 2023
8607581
Resolve review 5 remarks (rename sum_amounts to sum, add Copy for Ass…
dmidem May 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
124 changes: 37 additions & 87 deletions src/issuance.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand All @@ -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<T: IssueAuth> {
Expand All @@ -33,46 +35,6 @@ pub struct IssueBundle<T: IssueAuth> {
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<AssetBase, AssetSupply>,
}

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).
Expand Down Expand Up @@ -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
///
Expand All @@ -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(
Expand All @@ -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()))))
Expand Down Expand Up @@ -381,7 +339,7 @@ impl IssueBundle<Prepared> {
// 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(|_| ()))?;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm open to suggestions on how to avoid .map(|_| ())

Copy link
Author

@dmidem dmidem Apr 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see two options.

  1. Naive suggestion: we can use the ? operator to return early on an error and then return Ok(()) if the verification succeeds:
self.actions
    .iter()
    .try_for_each(|action| {
        action.verify_supply(&expected_ik)?;
        Ok(())
    })?;
  1. Alternatively, we can rename the verify_supply function to compute_supply, as this name more accurately reflects its purpose. And here we can add a comment to make it clear the successful result is intentionally not used:
// Make sure the `expected_ik` matches the `asset` for all notes.
// To do that, simulate supply computation for the sake of verification
// (ignore the successful result of `compute_supply`).
self.actions
    .iter()
    .try_for_each(|action| action.compute_supply(&expected_ik).map(|_| ()))?;

The second option looks better to me. And, of course, we can combine both fixes:

// Make sure the `expected_ik` matches the `asset` for all notes.
// To do that, simulate supply computation for the sake of verification
// (ignore the successful result of `compute_supply`).
self.actions
    .iter()
    .try_for_each(|action| {
        action.compute_supply(&expected_ik)?;
        Ok(())
    })?;

Copy link
Author

@dmidem dmidem Apr 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, do we need to perform all verification checks here or only check for uniqueness of asset base?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tnx for the 2 options, I prefer to keep the verify_supply() name. Let's go with

        // Make sure the `expected_ik` matches the `asset` for all notes.
        self.actions
            .iter()
            .try_for_each(|action| {
                action.verify_supply(&expected_ik)?;
                Ok(())
            })?;

No extra text for the comment is needed.


Ok(IssueBundle {
ik: self.ik,
Expand Down Expand Up @@ -461,9 +419,10 @@ pub fn verify_issue_bundle(
sighash: [u8; 32],
finalized: &mut HashSet<AssetBase>, // The finalization set.
) -> Result<SupplyInfo, Error> {
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
Expand All @@ -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(
Expand Down Expand Up @@ -607,7 +564,7 @@ mod tests {
(rng, isk, ik, recipient, sighash)
}

fn setup_compute_new_supply_test_params(
fn setup_verify_supply_test_params(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move to bottom together with the 3 new tests, the basic tests should come first.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

asset_desc: &str,
note1_value: u64,
note2_value: u64,
Expand Down Expand Up @@ -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());

Expand All @@ -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());

Expand All @@ -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)
);
}
Expand Down
1 change: 1 addition & 0 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ mod constants;
pub mod issuance;
pub mod keys;
pub mod note;
mod supply_info;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be pub like the rest

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

// pub mod note_encryption; // disabled until backward compatability is implemented.
pub mod note_encryption_v3;
pub mod primitives;
Expand Down
99 changes: 99 additions & 0 deletions src/supply_info.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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<AssetBase, AssetSupply>,
}

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();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somthing strange here, why we need .get_mut() if entry is already mut?

Copy link
Author

@dmidem dmidem Apr 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, I am using the entry() method now to avoid calling both get and insert. There is a more elegant way to work with entry result, using and_update and or_insert, but and_update does not allow returning an error. So, for our current logic, using a match with entry is the optimal choice. get_mut does require a mutable self reference, so entry needs to be mutable. However, in this particular case, we only need to use entry once, so we can convert it into AssetSupply using the into_mut method:

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;
}

I have fixed it now.

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();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this project, there are many custom types. Let's avoid default() and add an explicit new() constructor which is well defined in the scope of our new code.

Copy link
Author

@dmidem dmidem Apr 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's recommended by Clippy:

error: you should consider adding a `Default` implementation for `SupplyInfo`
  --> src/supply_info.rs:35:5
   |
35 | /     pub fn new() -> Self {
36 | |         Self {
37 | |             assets: HashMap::new(),
38 | |         }
39 | |     }
   | |_____^
   |
   = note: `-D clippy::new-without-default` implied by `-D warnings`
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#new_without_default
help: try adding this
   |
34 + impl Default for SupplyInfo {
35 +     fn default() -> Self {
36 +         Self::new()
37 +     }
38 + }
   |

To avoid the warning but still use new, we can either disable the Clippy warning:

#[allow(clippy::new_without_default)]
pub fn new() -> Self {
    Self {
        assets: HashMap::new(),
    }
}

Or add a default implementation as it recommends:

impl Default for SupplyInfo {
    fn default() -> Self {
        Self::new()
    }
}

For now, I've fixed it by using the second option (added the default implementation that refers to new). And so I use new in the rest of code instead of 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))
);
}
}