Refactor AssetSupply to AssetRecords and update issuance verification functions

[dmidem]

This PR modifies the verify_issue_bundle function and refactors the supply_info.rs module by removing SupplyInfo and introducing a new generic type, AssetInfo, in place of the previous AssetSupply struct. AssetInfo allows to properly distinguish the asset verification logic between reference notes required for first issuances versus optional references for subsequent issuances.

Key Changes

1. Rename AssetSupply struct to AssetInfo

2. Rename IssueAction::verify_supply to IssueAction::verify and update its return value

   • The verify method now returns only the sum of the action's note values instead of AssetInfo.
   • The is_finalized and reference_note action properties are now handled directly in the verify_issue_bundle function.

3. Revise verify_issue_bundle

   • Modifies the function signature to use a get_global_asset_state callback instead of relying on a finalization set.
   • Changes the return type to a HashMap<AssetBase, AssetInfo> (instead of SupplyInfo).
   • Updates the function so that a reference note is mandatory for the first appearance of any asset. Tests and documentation have been updated accordingly.

4. Remove supply_info.rs

   • Deletes the SupplyInfo struct and related tests, since verify_issue_bundle now returns a HashMap of AssetInfo objects.

5. Test updates

   • Revises tests to align with the new verify_issue_bundle signature and the get_global_asset_state approach.

6. Add issuance workflow test

   • Adds a new test issue_bundle_verify_with_global_state that performs a series of bundle creations and verifications, with a global state simulation.

7. Miscellaneous fixes

   • Simplifies Error variant imports in issuance.rs by referencing the local Error enum directly.

[PaulLaux]

Very good.

Added some semantic coments and one significant name change AssetInfo -> AssetRecord

Also, for get_action_by_asset() please fix comment to

/// # Returns
///
/// Returns `Some(&IssueAction)` if a single matching action is found.
/// Returns `None` if no action matches the given asset base.

(It is not very consise to start with If here)

Also, Add to the top of the file:

//! Issuance logic for Zcash Shielded Assets (ZSAs).
//!
//! This module defines structures and methods for creating, authorizing, and verifying  
//! issuance bundles, which introduce new shielded assets into the Orchard protocol.  
//!
//! The core components include:
//! - `IssueBundle`: Represents a collection of issuance actions with authorization states.
//! - `IssueAction`: Defines an individual issuance event, including asset details and notes.
//! - `IssueAuth` variants: Track issuance states from creation to finalization.
//! - `verify_issue_bundle`: Ensures issuance validity and prevents unauthorized asset creation.
//!
//! Errors related to issuance, such as invalid signatures or supply overflows,  
//! are handled through the `Error` enum.

[PaulLaux]

let's move this check (here and code) inside verify()

[dmidem]

Done.

[PaulLaux]

very good.
I did renames to simplify and adjust to the new record terminology. Semantics are the same:

pub fn verify_issue_bundle(
    bundle: &IssueBundle<Signed>,
    sighash: [u8; 32],
    get_global_records: impl Fn(&AssetBase) -> Option<AssetRecord>,
) -> Result<HashMap<AssetBase, AssetRecord>, Error> {
    bundle
        .ik()
        .verify(&sighash, bundle.authorization().signature())
        .map_err(|_| IssueBundleInvalidSignature)?;

    let new_records =
        bundle
            .actions()
            .iter()
            .try_fold(HashMap::new(), |mut new_records, action| {
                if !is_asset_desc_of_valid_size(action.asset_desc()) {
                    return Err(WrongAssetDescSize);
                }

                let (asset, amount) = action.verify(bundle.ik())?;
                let is_finalized = action.is_finalized();
                let ref_note = action.get_reference_note();

                let new_asset_record = match new_records
                    .get(&asset)
                    .cloned()
                    .or_else(|| get_global_records(&asset))
                {
                    // The first issuance of the asset
                    None => AssetRecord::new(
                        amount,
                        is_finalized,
                        *ref_note.ok_or(MissingReferenceNoteOnFirstIssuance)?,
                    ),

                    // Subsequent issuances of the asset
                    Some(current_record) => {
                        let amount =
                            (current_record.amount + amount).ok_or(ValueOverflow)?;

                        if current_record.is_finalized {
                            return Err(IssueActionPreviouslyFinalizedAssetBase(asset));
                        }

                        AssetRecord::new(amount, is_finalized, current_record.reference_note)
                    }
                };

                new_records.insert(asset, new_asset_record);

                Ok(new_records)
            })?;

    Ok(new_records)
}

[dmidem]

Done (updated the function).

[PaulLaux]

Consider defining a proper function here for clerity

[dmidem]

Fixed (converted it to a function).

[PaulLaux]

Good,
One more iteration is needed to finilize.
Also, please move is_reference_note() down, under create_referance_note()

[PaulLaux]

Subsequent issuance (issuance is already plural)

[dmidem]

Fixed.

[PaulLaux]

consider

        fn get_first_note(bundle: &IssueBundle<Signed>, action_index: usize) -> &Note {
            bundle.actions()[action_index].notes().first().unwrap()
        }

better naming + you are cloning the notes anyway later.

[dmidem]

Fixed (renamed).

[PaulLaux]

consider &Note. clone inside, if needed. Aslo for compatibility with get_first_note()->&Note

[PaulLaux]

also fn build_global_state()

[dmidem]

I refactored the code and removed the build_expected_global_state function. Now, I simply use HashMap::from([...]) instead, passing an array into it. To create the array elements, I added a simple build_state_entryfunction (forNote- it receives it by reference, i.e.&Note`).

[PaulLaux]

redundent, just collect()

[dmidem]

I refactored the code and use HashMap::from([...]) now, as mentioned above.

[PaulLaux]

*reference_note

[dmidem]

Fixed it (in new build_state_entry function, see comments above).

[PaulLaux]

replace the tuple with a proper data struct and a constructor.
The bool,bool is very confusing

[dmidem]

Fixed - added BundleTestData struct for that with a constructor.

[PaulLaux]

we always do asset_desc, amount, is_finilized, .... Let's stick to this order

[dmidem]

Fixed (in the new BundleTestData struct I mentioned above).

[PaulLaux]

why params is needed? Let's just go with named input params.

fn build_issue_bundle(
    rng: OsRng,
    isk: IssuanceAuthorizingKey,
    ik: IssuanceValidatingKey,
    recipient: Address,
    sighash: [u8; 32],
    first_nullifier: Nullifier,
    note_list: &[TestNoteList],
) -> IssueBundle<Signed> {

(it does not make sence to preserve the un-named tuple inside the function)

[PaulLaux]

I see that it is based on the old setup_params() function but complexity has grown since then.
(sighash and first_nullifer cannot be identified by their type)

Two ways forward:

1. continue using the setup_params() but dcompose it into named params before calling the new build_issue_bundle()

2. change setup_params to return a struct and use the struct as an input to `build_issue_bundle.

Anyway we should have names to the params.

[dmidem]

Fixed - I introduced new struct called TestParams with those fields, so setup_params returns that struct now instead of a tuple. Updated all tests to use it (replaced tuple destructuring with struct destructuring).

[PaulLaux]

Good restructure, added comments to finilize

[PaulLaux]

Add a comment that these parameters are for testing global state only and should not be used in an actual setting.

[dmidem]

Done.