Implement `send_note` for basic wallet #808

Fumuran · 2024-07-30T15:41:58Z

This PR implements a new send_note procedure for basic wallet.
This procedure created a new note and adds an arbitrary number of assets to it. Assets are provided to the procedure by their number and a memory address where they are stored.

TODO:

Update docs
Update CHANGELOG
Create an issue: remove send_asset wallet procedure

Fumuran · 2024-07-30T15:57:36Z

During debugging I noticed that we have a mismatch in the description of how create_note kernel procedure should work. Instead of returning note_idx and 5 zeros, as it specified in the description, this procedure returns note_idx and 6 zeros.

I can't see it causing any problems in our implementations of whatever uses this procedure, but in my case I had to remove excess zero to make my procedure work. Probably we should create an issue to investigate further and at least update the docs.

Fumuran · 2024-07-30T16:00:00Z

Current version of the test for the send_note procedure doesn't work. To make it pass I needed to somehow provide the values representing the assets to the new context where send_note is executed, which, for now, I don't know how to do.

bobbinth

Looks good! Thank you! I left a few comments inline. But also, I'm wondering if the approach w'eve taken here is the right one. An alternative approach could be:

We add a procedure send_note which creates a note without assets (i.e., no need to pass assets into this function).
We add a procedure move_asset_into_note. This procedure takes a note index and an asset, removes the specified asset from the account and adds it to the note.

Then the caller could use these two procedures to create notes with however many assets they need, and i would say that it shouldn't be any more complicated than the current approach (though, it may be a bit less efficient).

We could also potentially get rid of the send_asset procedure as these two procedures should be able to replace it.

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

Fumuran · 2024-08-02T07:05:25Z

An alternative approach could be:

We add a procedure send_note which creates a note without assets (i.e., no need to pass assets into this function).

We add a procedure move_asset_into_note. This procedure takes a note index and an asset, removes the specified asset from the account and adds it to the note.

I agree, once the procedure needs an internal delimiter comments, it is the sign to divide it. I'll implement the move_asset_into_note to lighten the send_note.

bobbinth

Thank you! Looks good. I left a coupe more comments inline - overall, I think we should simplify this quite a bit and also try to introduce ABI described in #685.

Also, should we try to fix the inconsistency mentioned in #808 (comment)?

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

bobbinth

Thank you! Looks good. Not a full review yet - but I left some more comments inline.

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

bobbinth

Looks good! Thank you! I reviewed pretty much everything except for the new test and left some more comments inline.

bobbinth · 2024-08-08T18:33:03Z

miden-lib/asm/miden/tx.masm

 export.add_asset_to_note
+    dupw movup.8
+    # => [note_idx, ASSET, ASSET]
+
    syscall.add_asset_to_note
-    # => [note_idx, EMPTY_WORD]
+    # => [note_idx, EMPTY_WORD, ASSET]

    # clear the padding from the kernel response
-    movdn.4 dropw
-    # => [note_idx]
+    movdn.8 dropw
+    # => [ASSET, note_idx]
 end


Could we also update add_asset_to_note in api.masm to have the same signature as here? This way, we won't need to do extra dupw and dropw here.

bobbinth · 2024-08-08T18:42:56Z

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

    # load the ASSET and add it to the note
-    padw loc_loadw.0 movup.4 exec.tx::add_asset_to_note
+    padw loc_loadw.0 exec.tx::add_asset_to_note dropw
    # => [note_idx, ASSET, EMPTY_WORD, ...]


Why do we need to first do padw and then dropw here? Could we not do something like:

movdn.4 loc_loadw.0 movup.4 exec.tx::add_asset_to_note

Hm, I think I made a mistake here, it slipped my mind that tx::add_asset_to_note have a [ASSET, note_idx] -> [ASSET, note_idx] stack transition, so we don't need to drop the word at the end. So something like this would work:

movdn.4 loc_loadw.0 exec.tx::add_asset_to_note movup.4

It is a problem though that this procedure was returning a wrong result, and no test was failing, although we have tests for distribute procedure. I'll check why this happened.

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

bobbinth

Thank you! I left a few more comments inline.

bobbinth · 2024-08-08T21:57:25Z

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

+#! This procedure is expected to be invoked using a `call` instruction. It makes no guarantees about
+#! the contents of the `PAD` elements shown below. It is the caller's responsibility to make sure
+#! these elements do not contain any meaningful data.


All exported procedures in this file are expected to be invoked using a syscall instruction. So, I'd update this comment and move it to the top of the file.

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

bobbinth · 2024-08-08T22:04:45Z

miden-lib/asm/miden/tx.masm

 #! Adds the ASSET to the note specified by the index.
 #!
-#! Inputs: [note_idx, ASSET]
-#! Outputs: [note_idx]
+#! This procedure is expected to be invoked using a `call` instruction. It makes no guarantees about
+#! the contents of the `PAD` elements shown below. It is the caller's responsibility to make sure
+#! these elements do not contain any meaningful data.
+#!
+#! Inputs:  [ASSET, note_idx, PAD(11)]
+#! Outputs: [ASSET, note_idx, PAD(11)]
 #!
 #! note_idx is the index of the note to which the asset is added.
 #! ASSET can be a fungible or non-fungible asset.
 export.add_asset_to_note


This procedure is invoked via exec instruction - not call instruction. So, the comment is not accurate here. And the signature for this procedure should probably be:

#! Inputs: [ASSET, note_idx, ...] #! Outputs: [ASSET, note_idx, ...]

Within this procedure, we should ensure that we pad and "unpad" the stack appropriately before making a syscall. That is, the stack before the syscall should look like this:

# => [ASSET, note_idx, ZERO(11)]

Unless, of course, we change the methodology to guarantee that add_asset_to_note in api.masm does not modify the PAD elements.

Fumuran · 2024-08-09T09:08:42Z

For now I applied the ABI rules just for the procedures that I used in the tests (tx::create_note, tx::add_asset_to_note, some procs in the tx script), I think it will be better to move the refactoring of the other procedures to the different (specific) PR.

miden-tx/src/tests/mod.rs

bobbinth

Looks good! Thank you! I left a few comments inline. After these are addressed, we can merged (though if #816 gets merged first, we'll need to update this PR).

For now I applied the ABI rules just for the procedures that I used in the tests (tx::create_note, tx::add_asset_to_note, some procs in the tx script), I think it will be better to move the refactoring of the other procedures to the different (specific) PR.

Agreed. I'm actually not 100% sure yet if we should adapt this ABI for kernel procedures as it results in a bit more stack manipulation than I'd like. The main problem is that without doing this, we need to guarantee that these procedures don't modify the elements they are not supposed to. If we had a good testing framework for that, this could be good enough to avoid these ABI changes.

bobbinth · 2024-08-09T19:25:30Z

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

@@ -8,6 +8,13 @@ use.miden::kernels::tx::memory
 use.miden::kernels::tx::note
 use.miden::kernels::tx::tx

+# NOTE
+# =================================================================================================
+# Procedures in this module are expected to be invoked using a `call` instruction. It makes no    #


We should change call to syscall as all procedures in this module are be accessible via syscalls only.

bobbinth · 2024-08-09T19:37:39Z

miden-lib/asm/miden/tx.masm

+    push.0 movdn.7 padw padw movdnw.3 movdnw.3
+    # => [tag, aux, note_type, RECIPIENT, PAD(9)]


I think we should be able to do something like this here:

push.0 movdn.7 padw padw swapdw

(movdnw instructions are more expensive than swapdw).

Though, it will become just padw padw swapdw after #816 is merged.

bobbinth · 2024-08-09T19:45:50Z

miden-lib/asm/miden/tx.masm

-    # clear the padding from the kernel response
-    movdn.4 dropw swap drop
+    # remove excess PADs from the stack 
+    movdnw.3 dropw dropw dropw movdn.3 drop drop drop


Here too I think we can use swapdw to make things a bit more efficient:

swapdw dropw dropw movdn.8 dropw drop drop drop

bobbinth · 2024-08-09T19:46:53Z

miden-lib/asm/miden/tx.masm

+    # elements 
+    push.0.0.0 padw padw movupw.3 movup.15 movup.4
+    # => [note_idx, ASSET, PAD(11)]

-    # clear the padding from the kernel response
-    movdn.4 dropw
-    # => [note_idx]
+    syscall.add_asset_to_note 
+    # => [note_idx, ASSET, PAD(11)]
+
+    # remove excess PADs from the stack 
+    movdn.15 movdnw.3 dropw dropw drop drop drop movdn.4


Similar comments here about potentially making things a bit more efficient via swapdw instruction.

Fumuran requested a review from bobbinth July 30, 2024 16:00

Fumuran force-pushed the andrew-impl-send-note branch 2 times, most recently from 52a8a9d to 34492d8 Compare July 31, 2024 19:16

bobbinth reviewed Aug 2, 2024

View reviewed changes

Fumuran force-pushed the andrew-impl-send-note branch from 720f6c9 to 2728521 Compare August 2, 2024 18:33

Fumuran marked this pull request as ready for review August 2, 2024 18:42

Fumuran requested a review from bobbinth August 2, 2024 18:42

bobbinth requested changes Aug 2, 2024

View reviewed changes

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

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

Fumuran force-pushed the andrew-impl-send-note branch 2 times, most recently from da44180 to 52df279 Compare August 6, 2024 07:12

Fumuran requested a review from bobbinth August 7, 2024 08:18

Fumuran mentioned this pull request Aug 7, 2024

Remove send_asset procedure from the basic wallet. #821

Closed

bobbinth requested changes Aug 8, 2024

View reviewed changes

Fumuran force-pushed the andrew-impl-send-note branch from bb05603 to 61e419d Compare August 8, 2024 13:08

Fumuran requested a review from bobbinth August 8, 2024 13:16

bobbinth reviewed Aug 8, 2024

View reviewed changes

Fumuran requested a review from bobbinth August 8, 2024 21:04

bobbinth reviewed Aug 8, 2024

View reviewed changes

Fumuran force-pushed the andrew-impl-send-note branch from df2d1c7 to 3b264d6 Compare August 9, 2024 09:08

Fumuran force-pushed the andrew-impl-send-note branch from 3b264d6 to 197be54 Compare August 9, 2024 09:15

Fumuran commented Aug 9, 2024

View reviewed changes

miden-tx/src/tests/mod.rs Outdated Show resolved Hide resolved

Fumuran requested a review from bobbinth August 9, 2024 09:22

bobbinth approved these changes Aug 9, 2024

View reviewed changes

Fumuran added 3 commits August 9, 2024 23:37

feat: implement send_note procedure

8f302c6

test: attempt to implement test for send_note

6cd8958

feat: send_note without hash check

058efec

Fumuran added 9 commits August 9, 2024 23:37

feat: implement send_note with hash check

1f643fe

refactor: divide send_note into two procedures

fb3f2d9

chore: update changelog, fix formatting in bench_tx/README

3d9ca56

refactor: simplify move_asset_to_note

5f295e8

refactor: change add_asset_to_note procedure output, update tests

9b176ec

chore: update CHANGELOG

3319b17

refactor: optimize add_asset_to_note, update comments

b10e77a

refactor: update comments, update padding for called and syscalled procs

a8a3aab

refactor: update padding before calls

9e3849c

Fumuran force-pushed the andrew-impl-send-note branch from 197be54 to 9e3849c Compare August 9, 2024 20:38

Fumuran merged commit 77f0f74 into next Aug 9, 2024
13 checks passed

Fumuran deleted the andrew-impl-send-note branch August 9, 2024 20:47

Fumuran mentioned this pull request Aug 9, 2024

feat: add a procedure for basic wallets that creates notes with 0 or more assets #800

Closed

bobbinth mentioned this pull request Aug 9, 2024

feat: Encode NoteExecutionHint into NoteMetadata #816

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `send_note` for basic wallet #808

Implement `send_note` for basic wallet #808

Fumuran commented Jul 30, 2024 •

edited

Loading

Fumuran commented Jul 30, 2024

Fumuran commented Jul 30, 2024

bobbinth left a comment

Fumuran commented Aug 2, 2024

bobbinth left a comment

bobbinth left a comment

bobbinth left a comment

bobbinth Aug 8, 2024

bobbinth Aug 8, 2024

Fumuran Aug 8, 2024

bobbinth left a comment

bobbinth Aug 8, 2024

bobbinth Aug 8, 2024

Fumuran commented Aug 9, 2024

bobbinth left a comment

bobbinth Aug 9, 2024

bobbinth Aug 9, 2024

bobbinth Aug 9, 2024

bobbinth Aug 9, 2024

		push.0 movdn.7 padw padw movdnw.3 movdnw.3
		# => [tag, aux, note_type, RECIPIENT, PAD(9)]

Implement send_note for basic wallet #808

Implement send_note for basic wallet #808

Conversation

Fumuran commented Jul 30, 2024 • edited Loading

Fumuran commented Jul 30, 2024

Fumuran commented Jul 30, 2024

bobbinth left a comment

Choose a reason for hiding this comment

Fumuran commented Aug 2, 2024

bobbinth left a comment

Choose a reason for hiding this comment

bobbinth left a comment

Choose a reason for hiding this comment

bobbinth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobbinth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fumuran commented Aug 9, 2024

bobbinth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Implement `send_note` for basic wallet #808

Implement `send_note` for basic wallet #808

Fumuran commented Jul 30, 2024 •

edited

Loading