feat: add assets to a note #614

Dominik1999 · 2024-04-18T20:50:59Z

Adding the ability to add assets to a note.

I changed the kernel procedures for tx::create_note and I added the procedure tx::add_asset_to_note. That means, first a note is being created without any assets, then, assets can be added.
Also, I added the aux_data field to tx::create_note and subsequent changes.
Then, the way how we create OutputNotes in the Host needed to change. We now collect all the note data during execution, because we can always add new assets, and build the notes at the end.

hackaugusto · 2024-04-18T21:12:16Z

Have you considered also doing #563 ? Another alternative would be to give an asset counter and a list of assets. I'm not sure how to provide the list of assets though, AFAIK we can only have 16 elements in the stack, so it would need to go somewhere else, one possibility is to push them to the advice provider and then give the hash of the assets to the create_note call (well, in this case with the hash, the counter can be put as value for the same key).

Dominik1999 · 2024-04-19T09:02:33Z

That is an excellent point; maybe I should rather work on #563.

One problem is that tx::create_note calls syscall.create_note. This changes the context, and we can only add max two ASSETs since the procedure already takes [tag, note_type, RECIPIENT].

That means, instead of providing the full ASSET, we should provide the ROOT of a tree of ASSETS that go into a specific note. If we add a num_of_assets to the create_note procedure, we can iterate over the tree and use numbers as KEYs. At KEY 0 we can store the note_idx to ensure we put the correct ASSETs into the right note.

But the overall complexity grows quite a bit. I mean, you need to define before the transaction which assets you want to send with which note. That has implications for the note_args use case as well.

#! Creates a new note and returns a pointer to the memory address at which the note is stored.
#!
#! Inputs: [num_of_assets, ROOT, tag, note_type, RECIPIENT]
#! Outputs: [ptr]
#!
#! num_of_assets is how many different assets the note should contain
#! ROOT is the root of the tree of all ASSETs to send stored in memory
#! tag is the tag to be included in the note.
#! note_type is the storage type of the note
#! RECIPIENT is the recipient of the note.
#! ptr is the pointer to the memory address at which the note is stored.
export.create_note
    syscall.create_note
    # => [ptr, ZERO, ZERO, 0]

    # clear the padding from the kernel response
    movdn.8 dropw dropw swap drop
    # => [ptr]
end

bobbinth · 2024-04-19T09:14:34Z

Maybe the way to go here is to change how create_note works so that it takes no assets. Then, we could add assets one by one to the note through something like #228.

This would also address #563.

Dominik1999 · 2024-04-19T13:32:55Z

that way we could address #563 #228 and #596.

That might be the easiest to simply redefine:

#! Creates a new note and returns a pointer to the memory address at which the note is stored.
#!
#! Inputs: [aux, tag, note_type, RECIPIENT]
#! Outputs: [ptr]
#! ...
export.create_note
    syscall.create_note
    # => [ptr, ZERO, 0]
end

#! Adds an asset to a note
#!
#! Inputs: [note_idx or note_ptr, ASSET]
#! Outputs: [ptr]
#! ...
export.add_asset_to_note
    syscall.add_asset_to_note
    # => [ptr, ZERO, 0]
end

That might be quite a change. Let me check.

Dominik1999 · 2024-04-30T14:57:38Z

miden-lib/asm/miden/tx.masm

+#!
+#! ptr is the pointer to the memory address at which the note is stored.
+#! ASSET can be a fungible or non-fungible asset.
+export.add_asset_to_note


@bobbinth @hackaugusto actually here, we want to use

exec.account::remove_asset to remove the asset before we put it on the note.

Right now, this is blocked by our MockDataStore which is the basis for all our tests.

We need to refactor that first, before we can make create_note cleaner.

This is a question for @bobbinth . I don't know why create_note doesn't call remove_asset. It seems the user is expected to manually handle these details.

miden-lib/asm/miden/contracts/wallets/basic.masm

Dominik1999 · 2024-04-30T14:59:13Z

miden-tx/src/host/note_tracker.rs

+// OUTPUT NOTE BUILDER
+// ================================================================================================
+#[derive(Debug, Clone, Eq, PartialEq)]
+pub struct OutputNoteData {


Not sure if this is the most elegant solution. Maybe a normal dict would have sufficed?

What do you mean? Why do you want to use a map here?

Dominik1999 · 2024-04-30T14:59:46Z

miden-tx/src/host/mod.rs

@@ -305,6 +314,50 @@ impl<A: AdviceProvider> TransactionHost<A> {
            Ok(process.get_mem_value(process.ctx(), note_address).map(NoteId::from))
        }
    }
+
+    /// ToDo: pretty ugly function, refactor


It works, but is pretty ugly.

hackaugusto · 2024-04-30T21:26:23Z

miden-lib/asm/kernels/transaction/api.masm

+    # prepare the stack for return
+    #padw push.0 push.0 movup.6 
+    # => [ptr, 0, 0, 0, 0, 0, 0]


I think we have issues with the stack handling in this file.

Procedures inside the asm/kernels folder are what form the kernel interface. This means the procedures in this file are sycalled (create_note is called here ). Because this procedure is used via a syscall, there are exactly 16 elements in the stack as inputs, and there must be exactly 16 elements in the stack as outputs.

When entering this procedure, the stack looks like this:

[tag, aux, note_type, RECIPIENT, x, X, X]

Where x, X, X are 9 elements which may be important for the caller, so we don't want to lose them, or change their position.

Without padding the will look like this after the exec.tx::create_note

[ptr, x, X, X, 0, 0, ZERO]

The 0, 0, ZERO at the end are automatically added by the VM, because the stack always has a minimum of 16 elements. Note that the position of the x, X, X changed, that is what we want to prevent. One solution is just to move the elements back to its original position, something like:

movup.10 movup.11 # => [0, 0, ptr, x, X, X, ZERO] swapw.2 swapw.3 # => [ZERO, X, 0, 0, ptr, x, X] swapw swapw.2 # => [0, 0, ptr, x, ZERO, X, X] movup.3 swap.7 # => [0, 0, ptr, ZERO, x, X, X] movup.2 # => [ptr, 0, 0, ZERO, x, X, X]

Or, another alternative is to pad the with 0, 0, ZERO before doing exec.tx::create_note, that would be something like:

exec.authenticate_account_origin # => [tag, aux, note_type, RECIPIENT, x, X, X] push.0 padw # => [ZERO, 0, tag, aux, note_type, RECIPIENT, x, X, X] swapw.2 # => [0, tag, aux, note_type, RECIPIENT, ZERO, x, X, X] movdn.7 push.0 movdn.7 # => [tag, aux, note_type, RECIPIENT, 0, 0, ZERO, x, X, X]

So after the exec.tx::create_note the stack will look like:

# => [ptr, 0, 0, ZERO, x, X, X]

I guess we don't have any tests that call create_note and check the stack elements that would be x, X, X, like we talked here

Thanks for the in-depth explanation.

I changed the asm/api.masm create_note procedure now to

export.create_note # authenticate that the procedure invocation originates from the account context exec.authenticate_account_origin # => [tag, aux, note_type, RECIPIENT] exec.tx::create_note # => [ptr] # prepare the stack for return movupw.3 movup.15 movup.15 movup.6 # => [ptr, 0, 0, 0, 0, 0, 0] end

This should do the job.

When entering the procedure, we have

[tag, aux, note_type, RECIPIENT, x, X, X]

after exec.tx::create_note we should have (without padding)

[ptr, x, X, X, 0, 0, ZERO]

so we can do

movupw.3 movup.15 movup.15 movup.6

to reach

[ptr, 0, 0, ZERO, x, X, X]

I think we should document the changes to the stack in the source code too. It really helps to understand the instructions movupw.3 movup.15 movup.15 movup.6.

# => [ptr, x, X, X, 0, 0, ZERO] movupw.3 # => [ZERO, ptr, x, X, X, 0, 0] movup.15 movup.15 # => [0, 0, ZERO, ptr, x, X, X] movup.6 # => [ptr, 0, 0, ZERO, x, X, X]

Really neat solution btw.

I added some comments

Really neat solution btw

thanks, ser

hackaugusto · 2024-04-30T21:26:29Z

miden-lib/asm/kernels/transaction/api.masm

+    # prepare stack for return
+    #padw movup.4
+    # => [ptr, 0, 0, 0, 0]


Same here, solved by

movupw.3 movup.4

hackaugusto · 2024-04-30T21:38:31Z

miden-lib/asm/miden/contracts/faucets/basic_fungible.masm

    # check that amount =< max_supply - total_issuance, fails if otherwise
    dup.1 gte assert.err=ERR_BASIC_FUNGIBLE_MAX_SUPPLY_OVERFLOW
-    # => [asset, tag, note_type, RECIPIENT, ...]
+    # => [asset, tag, aux, note_type, RECIPIENT, ...]


Not introduced by you, but this logic seems duplicated with the exec.faucet::mint below:

miden-base/miden-lib/asm/miden/kernels/tx/faucet.masm

Lines 54 to 60 in 7e1db46

# prepare stack to ensure that minting the asset will not exceed the maximum

dup.7 dup exec.asset::get_fungible_asset_max_amount dup.3

# => [total_issuance, max_allowed_issuance, amount, amount, TOTAL_ISSUANCE, ASSET]

# compute difference to ensure that the total issuance will not exceed the maximum

sub lte assert.err=ERR_FAUCET_ISSUANCE_OVERFLOW

# => [amount, TOTAL_ISSUANCE, ASSET]

indeed, we should be able to delete that line here. Don't you think?

Ah no, I think those are two different checks.

The first check in the contract, checks against the max_supply, set by the user at account creation.

The second check, in asset.masm checks against the total max number of any asset const.FUNGIBLE_ASSET_MAX_AMOUNT=9223372036854775807

You are right, that in theory, the first check, should ensure that the max supply provided at slot 1 in the faucet is smaller than 9223372036854775807, which is checked at account creation. But an additional check doesn't hurt for faucets.

Sounds good 👍 . Lets maybe add a comment explaining the difference?

miden-lib/asm/miden/contracts/faucets/basic_fungible.masm

…639)

miden-lib/asm/miden/contracts/wallets/basic.masm

miden-lib/asm/miden/kernels/tx/asset.masm

hackaugusto · 2024-05-02T15:00:52Z

miden-tx/src/host/note_tracker.rs

+// OUTPUT NOTE BUILDER
+// ================================================================================================
+#[derive(Debug, Clone, Eq, PartialEq)]
+pub struct OutputNoteData {


What do you mean? Why do you want to use a map here?

objects/src/notes/assets.rs

miden-lib/asm/miden/contracts/wallets/basic.masm

hackaugusto · 2024-05-02T15:17:48Z

miden-lib/asm/miden/tx.masm

+#!
+#! ptr is the pointer to the memory address at which the note is stored.
+#! ASSET can be a fungible or non-fungible asset.
+export.add_asset_to_note


This is a question for @bobbinth . I don't know why create_note doesn't call remove_asset. It seems the user is expected to manually handle these details.

bobbinth

Looks good! Thank you! I left some comments with improvement suggestions inline. One thing I didn't really get deep into is the handling of the padding in various functions. I'll do a more thorough review of these on the next pass.

miden-tx/src/host/mod.rs

bobbinth · 2024-05-06T06:29:38Z

miden-lib/asm/miden/contracts/faucets/basic_fungible.masm

 #! Distributes freshly minted fungible assets to the provided recipient.
-#! Inputs: [amount, tag, note_type, RECIPIENT]
+#! Inputs: [amount, tag, aux, note_type, RECIPIENT]
 #! Outputs: [note_ptr, 0, 0, 0, 0, 0, 0, 0, 0, ...]


I'm not sure we need to change the public interface of this file (we could pass 0 for aux field internally).

bobbinth · 2024-05-06T06:35:26Z

miden-lib/asm/miden/contracts/faucets/basic_fungible.masm


    # check that amount =< max_supply - total_issuance, fails if otherwise
    dup.1 gte assert.err=ERR_BASIC_FUNGIBLE_MAX_SUPPLY_OVERFLOW
-    # => [asset, tag, note_type, RECIPIENT, ...]
+    # => [asset, tag, aux, note_type, RECIPIENT, ...]


Should asset here be actually amount?

bobbinth · 2024-05-06T06:38:50Z

miden-lib/asm/miden/contracts/faucets/basic_fungible.masm

+    exec.tx::move_asset_to_note
+    # => [note_ptr, ZERO, ...]


Is the returned stack here correct? I think we usually try to make sure that the number of elements on the stack doesn't change (i.e., should this not be [note_ptr, ZERO, ZERO]?

bobbinth · 2024-05-06T06:42:20Z

miden-lib/asm/miden/kernels/tx/tx.masm

 #! Creates a new note and returns a pointer to the memory address at which the note is stored.
 #!
-#! Inputs: [ASSET, tag, note_type, RECIPIENT]
-#! Outputs: [ptr, 0, 0, 0, 0, 0, 0, 0, 0]
+#! Inputs: [tag, aux, note_type, RECIPIENT]
+#! Outputs: [ptr]


What is the reason for removing zeros from the returned stack? Was it a mistake that we had them here before?

bobbinth · 2024-05-06T07:09:09Z

miden-lib/asm/miden/kernels/tx/tx.masm

+#! c, If there is no identical ASSET, the procedure returns the asset_ptr of the next 
+#!    available memory location.
+#! 
+#! Inputs: [ASSET, ptr, num_of_assets]


nit: I would probably rename ptr to note_ptr (here and in other places).

bobbinth · 2024-05-06T07:44:58Z

miden-lib/asm/miden/kernels/tx/tx.masm

+#! ASSET can be a fungible or non-fungible asset.
+export.move_asset_to_note


Now seeing how this works, I think add_asset_to_note would have been the correct name.

bobbinth · 2024-05-06T07:57:39Z

miden-lib/asm/miden/kernels/tx/tx.masm

+    movdn.4 exec.asset::validate_asset
+    # => [ASSET, ptr]

-    movdn.8 swapw padw swapw movup.12
-    # => [note_ptr, RECIPIENT, 0, 0, 0, 0, 0, 0, 0, 0]
+    # check if the note has at least one ASSET already
+    dup.4 exec.memory::get_created_note_num_assets dup movdn.6 neq.0
+    # => [num_of_assets>0?, ASSET, ptr, num_of_assets]
+
+    if.true
+        exec.check_if_asset_already_exists
+        # => [asset_ptr, ASSET'', ptr, num_of_assets]
+    else
+        # There is no asset in the note yet, so we add the asset at the first position
+        dup.4 exec.memory::get_created_note_asset_data_ptr
+        # => [asset_ptr, ASSET, ptr, num_of_assets]
+    end


I would have probably taken a slightly different approach here:

First, we could validate the asset (as you have now) and determine its type (fungible or non-fungible).

Then, for non-fungible assets, I'd create a helper procedure - e.g., add_non_fungible_asset_to_note. In this function, we could use simple eqw comparisons in a loop, and then, at the end of the loop, if nothing failed, add the asset to the note. So, the function would be relatively simple.

For fungible assets, I'd create a separate helper procedure - e.g., add_fungible_asset_to_note. I'd also create two other helper procedures:

Something like asset::get_issuer which would return the faucet ID that issued the asset.

Something like asset::combine_fungible_assets which would combine two fungible assets that have the same origin.

I would then use these functions similar to how you have now.

While this will probably result in a bit more code - I think the code would be more encapsulated and easier to follow/maintain (e.g., if the structure of assets changes, we would only need to update the relevant helper functions).

Dominik1999 · 2024-05-10T19:19:53Z

Closing this in favor of #686 and #674

Dominik1999 requested review from hackaugusto and bobbinth April 18, 2024 20:51

Dominik1999 force-pushed the dominik_feat_notes_without_assets branch from 1c18db8 to 380f834 Compare April 24, 2024 12:24

bobbinth force-pushed the next branch from ab860b5 to 9d914fe Compare April 25, 2024 20:32

Dominik1999 force-pushed the dominik_feat_notes_without_assets branch 2 times, most recently from 65fb04c to c1b57df Compare April 26, 2024 09:21

Dominik1999 changed the title ~~feat: notes without assets~~ feat: add ability to assets to a note Apr 26, 2024

bobbinth force-pushed the next branch from 9d914fe to 0ddc139 Compare April 26, 2024 10:05

Dominik1999 force-pushed the dominik_feat_notes_without_assets branch from c1b57df to 3c5727c Compare April 26, 2024 11:48

feat: adding notes without assets

1fabcc9

Dominik1999 force-pushed the dominik_feat_notes_without_assets branch from 3c5727c to 1fabcc9 Compare April 26, 2024 11:59

some refactorings

7e1db46

Dominik1999 changed the title ~~feat: add ability to assets to a note~~ feat: add assets to a note Apr 29, 2024

Dominik1999 commented Apr 30, 2024

View reviewed changes

miden-lib/asm/miden/contracts/wallets/basic.masm Show resolved Hide resolved

Dominik1999 commented Apr 30, 2024

View reviewed changes

hackaugusto reviewed Apr 30, 2024

View reviewed changes

Dominik1999 requested a review from hackaugusto May 2, 2024 11:51

polydez and others added 4 commits May 2, 2024 14:03

feat: Implemented Block::build_note_tree (#630)

6870acd

refactor: changed types of version and timestamp fields to u32 (#…

fe331ca

…639)

feat: implement AccountStorageDeltaBuilder (#626)

711ff2a

after first feedback on the MASM files

990fa5f