Update AssetSuply and SupplyInfo (new) by dmidem · Pull Request #133 · QED-it/orchard

dmidem · 2025-01-07T08:48:09Z

This PR is an updated copy of #128:

The amount in AssetSupply must be a NoteValue (u64) and not a ValueSum (i128).
We add reference_notes into SupplyInfo. It is a hashmap of asset bases to their respective reference note

The code was updated, get_reference_note method of IssueAction was added and used, get_reference_notes was removed. Also, instead of a separate new reference_notes HashMap in SupplyInfo a new field reference_note was added to AssetSupply struct. Unit tests were fixed accordingly.

Co-authored-by: Dmitry Demin <dmidem@users.noreply.github.com>

…e type of the value was changed from ValueSum to NoteValue

…ntu package to be installed

…nfig Ubuntu package to be installed" This reverts commit c30e108.

…ich is required by the yeslogic-fontconfig-sys crate (a transitive dev dependency). The CI error started occurring after GitHub updated ubuntu-latest from Ubuntu 22.04 to 24.04.

…s field from SupplyInfo, add and use get_reference_note method to IssueAction

…t_reference_note, fix the tests accordingly

src/bundle/burn_validation.rs

ConstanceBeguier · 2025-01-07T10:13:05Z

src/bundle/burn_validation.rs

-        if !asset_set.insert(asset) {
-            return Err(BurnError::DuplicateAsset);
-        }
+    for (asset, value) in burn.into_iter().cloned() {


Is it necessary to clone the iterator?

The .cloned() method clones the iterating value, not the iterator itself. However, since we changed burn to be a slice, we don't need it now, so I removed it.

src/issuance.rs

src/supply_info.rs

src/value.rs

src/bundle/burn_validation.rs

…tion in burn_validation, supply_info, and issuance

PaulLaux

added comments, one is significant.

src/bundle/burn_validation.rs

PaulLaux · 2025-01-08T10:57:25Z

src/issuance.rs

+    pub fn get_reference_note(&self) -> Option<&Note> {
+        self.notes.iter().find(|note| {
+            (note.recipient() == ReferenceKeys::recipient()) && (note.value() == NoteValue::zero())
+        })
+    }


So according to the finalized spec, the reference note must be the first note.
The logic should be:

get first note

check if it is the ref note

if yes, return.

with .find() we might get a non deterministic behavior.

use the existing verify_reference_note()

So, if the first note doesn't match the reference note condition, it should return None even if, for example, there's the second note that does match the condition?

Now I changed it this way:

/// Returns the reference note. pub fn get_reference_note(&self) -> Option<&Note> { self.notes.first().filter(|note| { (note.recipient() == ReferenceKeys::recipient()) && (note.value() == NoteValue::zero()) }) }

It considers the first note only, and Option::filter additionally checks the referenence note condition for the first note.

filter is fine
but the logic to verify a referance note repeat itself many times: see verify_reference_note() x2 in tests.
please extract and use the extracted fn ->bool here and in both tests.

Also update doc for this function.

Now I added new function:

fn is_reference_note(note: &Note) -> bool { note.value() == NoteValue::zero() && note.recipient() == ReferenceKeys::recipient() }

and used it in get_reference_note and verify_reference_note, but verify_reference_note still has an additional check for asset matching, which does make sense for the tests, but looks duplicated for get_reference_note as we already checked this in IssueAction::verify_supply. So, I kept verify_reference_note as a separate function that calls is_reference_note and performs an additional check for the asset matching.

PaulLaux · 2025-01-08T11:05:19Z

src/supply_info.rs

    }

-    fn sum<'a, T: IntoIterator<Item = &'a AssetSupply>>(supplies: T) -> Option<ValueSum> {
+    fn sum<'a, T: IntoIterator<Item = &'a AssetSupply>>(supplies: T) -> Option<NoteValue> {


'a is not required:
fn sum<T: IntoIterator<Item = AssetSupply>>(supplies: T) -> Option<NoteValue> {

Removed 'a.

It seems that it is needed :/

src/supply_info.rs

…t note only

ConstanceBeguier · 2025-01-08T11:50:45Z

src/bundle/burn_validation.rs

+/// * Any asset in the `burn` vector has a zero value (`BurnError::ZeroAmount`).
+/// * Any asset in the `burn` vector is not unique (`BurnError::DuplicateAsset`).
+pub fn validate_bundle_burn(burn: &[(AssetBase, NoteValue)]) -> Result<(), BurnError> {
+    let mut burn_set = HashMap::new();


HashMap could be replaced by a HashSet with only the AssetBase inside it

…ests

PaulLaux

Very nice,
Please consider #133 (comment) and merge

ConstanceBeguier and others added 13 commits December 19, 2024 17:13

Update AssetSupply

2bee134

Add reference_notes into SupplyInfo

01eb3e3

Apply suggestions from code review

60e1854

Co-authored-by: Dmitry Demin <dmidem@users.noreply.github.com>

Move get_reference_notes to the first IssueBundle impl block

6c540dd

Rename ValueSumOverflow issuance error variant to ValueOverflow as th…

c9f8ba3

…e type of the value was changed from ValueSum to NoteValue

Pin yeslogic-fontconfig-sys to 5.0.0 as 6.0.0 requires fontconfig Ubu…

c30e108

…ntu package to be installed

Revert "Pin yeslogic-fontconfig-sys to 5.0.0 as 6.0.0 requires fontco…

9d1fcfa

…nfig Ubuntu package to be installed" This reverts commit c30e108.

Update burn_validation to use NoteValue instead of i64

19ee69f

Modify GitHub CI workflow to install libfontconfig1-dev on Ubuntu, wh…

4f43896

…ich is required by the yeslogic-fontconfig-sys crate (a transitive dev dependency). The CI error started occurring after GitHub updated ubuntu-latest from Ubuntu 22.04 to 24.04.

Use if runner.os in CI config instead of matrix.os check

a60428e

Merge branch 'zsa1' into update_asset_supply

d7973db

Add reference_note field to AssetSupply struct, remove reference_note…

2cce7db

…s field from SupplyInfo, add and use get_reference_note method to IssueAction

Remove get_reference_notes function as there's an action's menthod ge…

5b852f0

…t_reference_note, fix the tests accordingly

dmidem mentioned this pull request Jan 7, 2025

Update AssetSuply and SupplyInfo #128

Closed

Merge branch 'zsa1' into update_asset_supply

2bd20ab

dmidem requested review from ConstanceBeguier and PaulLaux January 7, 2025 08:50

QED-it deleted a comment from what-the-diff bot Jan 7, 2025

ConstanceBeguier reviewed Jan 7, 2025

View reviewed changes

Address review comments by updating function signatures and documenta…

062dfca

…tion in burn_validation, supply_info, and issuance

PaulLaux reviewed Jan 8, 2025

View reviewed changes

dmidem added 3 commits January 8, 2025 12:30

Minor fixes in burn_validation and supply_info.rs

c07e872

Changed IssueAction::get_reference_note method logic to take the firs…

1c7ef6c

…t note only

Return lifetime in supply info sum test

b3ed468

ConstanceBeguier reviewed Jan 8, 2025

View reviewed changes

dmidem added 2 commits January 8, 2025 13:10

Add is_reference_note function and use it in get_reference_note and t…

967cf84

…ests

Updated doc for get_reference_note

124dddf

PaulLaux approved these changes Jan 8, 2025

View reviewed changes

Use HashSet instead of HashMap in validate_bundle_burn

495c5d4

dmidem merged commit 69f92a3 into zsa1 Jan 8, 2025

PaulLaux mentioned this pull request May 14, 2025

enable webassembly support #164

Closed

Conversation

dmidem commented Jan 7, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PaulLaux left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

PaulLaux Jan 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmidem Jan 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmidem Jan 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmidem Jan 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ConstanceBeguier Jan 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PaulLaux left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

PaulLaux Jan 8, 2025 •

edited

Loading

dmidem Jan 8, 2025 •

edited

Loading

dmidem Jan 8, 2025 •

edited

Loading

dmidem Jan 8, 2025 •

edited

Loading

ConstanceBeguier Jan 8, 2025 •

edited

Loading