Note types #515

hackaugusto · 2024-03-07T17:56:10Z

Issue #402 , this follows the implementation described at #402 (comment)

This depends on #485

mock/src/mock/account.rs

polydez

Looks good to me! I've left one inline comment here.

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

bobbinth · 2024-03-11T08:56:10Z

@hackaugusto - could you rebase it from the latest next?

Also, in one of the other PRs (or issues), I left a comment about potentially reducing the note types to 3: OffChain, Public, and Encrypted. The main reasoning is that maybe we are over-complicating things and note type should specify only how the note data is stored (and maybe the proper name then would be NoteStorageMode). How the note should be consumed would be then be encoded in the note tag. This does require us to think though a bit more about how note tags would be structured. What do you think?

hackaugusto · 2024-03-11T10:06:35Z

How the note should be consumed would be then be encoded in the note tag. This does require us to think though a bit more about how note tags would be structured. What do you think?

I think that works, but I think it would be best to change the tag to be a structure, instead of encoding this in the existing field. Just to make things easier to understand.

bobbinth · 2024-03-12T00:08:22Z

@hackaugusto - could you rebase it from the latest next?

done

Seems like there are still some changes in this PR which were done in other PRs (e.g., introduction of ProvenTransactionBuilder).

bobbinth

Thank you! Not a full review - just left a few inline comments which I think may affect quite a few files.

objects/src/notes/note_type.rs

objects/src/notes/metadata.rs

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

hackaugusto · 2024-03-22T14:11:56Z

Thank you! Not a full review - just left a few inline comments which I think may affect quite a few files.

@bobbinth is there anything else you want to add to this PR? From what I gathered in the issue discussion the storage types are going to stay the same.

bobbinth · 2024-03-22T16:39:49Z

@bobbinth is there anything else you want to add to this PR? From what I gathered in the issue discussion the storage types are going to stay the same.

How are you thinking about tag-related changes - would these be for a different PR? (specifically, checking that the tag prefix is consistent with note type as discussed in #402)

hackaugusto · 2024-03-22T16:53:26Z

How are you thinking about tag-related changes - would these be for a different PR? (specifically, checking that the tag prefix is consistent with note type as discussed in #402)

Yes different PR.

bobbinth

Looks good! Thank you! I left some comments inline - most are nits but the main one is about letting callers of send_asset specify the type of the created note. Let me know what you think of this.

objects/src/notes/note_type.rs

objects/src/notes/mod.rs

objects/src/notes/metadata.rs

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

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

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

Dominik1999 · 2024-03-25T11:07:33Z

Can we merge this?

hackaugusto · 2024-03-25T11:58:38Z

Can we merge this?

I wouldn't merge it before @bobbinth has approved it.

hackaugusto · 2024-03-25T18:03:12Z

miden-lib/asm/miden/tx.masm


-    movdn.8 dropw dropw
+    # clear the padding from the kernel response
+    movdn.4 dropw movdn.4 dropw swap drop


I tried using locals, but the assembler fails with some error about mismatching number of locals.

You should be able to do movdn.8 dropw dropw swap drop and save 1 cycle.

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

miden-lib/asm/note_scripts/SWAP.masm

hackaugusto · 2024-03-25T18:05:26Z

miden-tx/src/tests.rs

+            push.{PUBLIC_NOTE}      # note_type
+            push.997                # tag
+            push.{REMOVED_ASSET_3}  # asset
+            call.wallet::send_asset dropw dropw drop drop


It seems that removing the dropw dropw drop drop would be a bigger issue, namely defining the calling convention. For now this makes the tests happy.

hackaugusto · 2024-03-25T18:06:27Z

@bobbinth once you have time, please re-review. I have finished the requested changes.

bobbinth

Thank you! Looks good! I left a few more comments inline - most of them are pretty small. The main one is about using syscall's from "user" code.

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

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

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

miden-lib/asm/note_scripts/SWAP.masm

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

bobbinth · 2024-03-25T21:04:33Z

miden-lib/asm/miden/tx.masm


-    movdn.8 dropw dropw
+    # clear the padding from the kernel response
+    movdn.4 dropw movdn.4 dropw swap drop


You should be able to do movdn.8 dropw dropw swap drop and save 1 cycle.

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

applied requested changes

Notes can be stored offchain, on-chain, or encrypted. This is an orthogonal concept to how the notes are executed. This commit adds a new metadata field to encode the storage type.

hackaugusto · 2024-03-26T18:47:19Z

fixed the CI

bobbinth

Looks good! Thank you!

I pushed a small commit which brings the changelog up to date. We can merge once CI completes.

hackaugusto requested review from bobbinth and polydez March 7, 2024 17:56

hackaugusto force-pushed the hacka-note-types branch from 60781d9 to 285be39 Compare March 7, 2024 17:59

hackaugusto changed the base branch from main to next March 7, 2024 18:00

hackaugusto force-pushed the hacka-note-types branch from 285be39 to 3e4720a Compare March 7, 2024 18:02

hackaugusto commented Mar 7, 2024

View reviewed changes

mock/src/mock/account.rs Outdated Show resolved Hide resolved

hackaugusto force-pushed the hacka-note-types branch from 3e4720a to 4a508bd Compare March 7, 2024 18:27

polydez approved these changes Mar 8, 2024

View reviewed changes

miden-lib/asm/kernels/transaction/api.masm Outdated Show resolved Hide resolved

hackaugusto force-pushed the hacka-note-types branch from 4a508bd to fde1b00 Compare March 8, 2024 14:25

bobbinth force-pushed the next branch from 5369f6b to 0156afd Compare March 8, 2024 23:06

hackaugusto force-pushed the hacka-note-types branch from fde1b00 to 1ca2c17 Compare March 11, 2024 10:26

This comment was marked as outdated.

Sign in to view

hackaugusto force-pushed the hacka-note-types branch 2 times, most recently from 9b8f48d to 10144c7 Compare March 12, 2024 16:46

bobbinth reviewed Mar 13, 2024

View reviewed changes

objects/src/notes/note_type.rs Outdated Show resolved Hide resolved

objects/src/notes/metadata.rs Show resolved Hide resolved

miden-lib/asm/miden/kernels/tx/tx.masm Outdated Show resolved Hide resolved

hackaugusto force-pushed the hacka-note-types branch from 10144c7 to 2e7b15f Compare March 13, 2024 16:14

bobbinth mentioned this pull request Mar 13, 2024

Note metadata and tag schemas #402

Closed

hackaugusto force-pushed the hacka-note-types branch 3 times, most recently from 716083e to ce2c98a Compare March 22, 2024 14:10

bobbinth reviewed Mar 22, 2024

View reviewed changes

hackaugusto force-pushed the hacka-note-types branch 4 times, most recently from 6ccafa2 to c73b70c Compare March 25, 2024 17:59

hackaugusto requested a review from bobbinth March 25, 2024 18:00

hackaugusto commented Mar 25, 2024

View reviewed changes

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

hackaugusto commented Mar 25, 2024

View reviewed changes

miden-lib/asm/note_scripts/SWAP.masm Show resolved Hide resolved

hackaugusto commented Mar 25, 2024

View reviewed changes

bobbinth previously requested changes Mar 25, 2024

View reviewed changes

hackaugusto force-pushed the hacka-note-types branch 3 times, most recently from a36fb78 to 6e20dba Compare March 26, 2024 17:09

hackaugusto requested a review from bobbinth March 26, 2024 17:10

hackaugusto force-pushed the hacka-note-types branch 3 times, most recently from 0a9b8bd to 73313c4 Compare March 26, 2024 17:31

note: implement note storage modes

3756f4a

Notes can be stored offchain, on-chain, or encrypted. This is an orthogonal concept to how the notes are executed. This commit adds a new metadata field to encode the storage type.

hackaugusto force-pushed the hacka-note-types branch from 73313c4 to 3756f4a Compare March 26, 2024 18:41

chore: update changelog

2519495

bobbinth approved these changes Mar 26, 2024

View reviewed changes

bobbinth merged commit cc2bec3 into next Mar 26, 2024
9 checks passed

bobbinth deleted the hacka-note-types branch March 26, 2024 19:53

bobbinth mentioned this pull request Apr 11, 2024

On-chain accounts and notes #403

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Note types #515

Note types #515

hackaugusto commented Mar 7, 2024 •

edited

Loading

polydez left a comment

bobbinth commented Mar 11, 2024

hackaugusto commented Mar 11, 2024

This comment was marked as outdated.

bobbinth commented Mar 12, 2024

This comment was marked as outdated.

bobbinth left a comment

hackaugusto commented Mar 22, 2024

bobbinth commented Mar 22, 2024

hackaugusto commented Mar 22, 2024

bobbinth left a comment

Dominik1999 commented Mar 25, 2024

hackaugusto commented Mar 25, 2024 •

edited

Loading

hackaugusto Mar 25, 2024

bobbinth Mar 25, 2024

hackaugusto Mar 26, 2024

hackaugusto Mar 25, 2024

hackaugusto commented Mar 25, 2024

bobbinth left a comment

bobbinth Mar 25, 2024

hackaugusto commented Mar 26, 2024

bobbinth left a comment

Note types #515

Note types #515

Conversation

hackaugusto commented Mar 7, 2024 • edited Loading

polydez left a comment

Choose a reason for hiding this comment

bobbinth commented Mar 11, 2024

hackaugusto commented Mar 11, 2024

This comment was marked as outdated.

bobbinth commented Mar 12, 2024

This comment was marked as outdated.

bobbinth left a comment

Choose a reason for hiding this comment

hackaugusto commented Mar 22, 2024

bobbinth commented Mar 22, 2024

hackaugusto commented Mar 22, 2024

bobbinth left a comment

Choose a reason for hiding this comment

Dominik1999 commented Mar 25, 2024

hackaugusto commented Mar 25, 2024 • edited Loading

hackaugusto Mar 25, 2024

Choose a reason for hiding this comment

bobbinth Mar 25, 2024

Choose a reason for hiding this comment

hackaugusto Mar 26, 2024

Choose a reason for hiding this comment

hackaugusto Mar 25, 2024

Choose a reason for hiding this comment

hackaugusto commented Mar 25, 2024

bobbinth left a comment

Choose a reason for hiding this comment

bobbinth Mar 25, 2024

Choose a reason for hiding this comment

hackaugusto commented Mar 26, 2024

bobbinth left a comment

Choose a reason for hiding this comment

hackaugusto commented Mar 7, 2024 •

edited

Loading

hackaugusto commented Mar 25, 2024 •

edited

Loading