Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add new get_script_hash kernel procedure #995

Merged
merged 11 commits into from
Dec 2, 2024
Merged

Conversation

tomyrd
Copy link
Collaborator

@tomyrd tomyrd commented Nov 29, 2024

closes #977

This PR adds a new get_script_hash kernel procedure that allows the user to access the note script hash from within the script itself.

@tomyrd
Copy link
Collaborator Author

tomyrd commented Nov 29, 2024

Some documentation is still missing but I just wanted to check if my approach was correct. It's a simple procedure but I'm not so experienced in this part of the codebase, so I created this draft first. I used the get_serial_number procedure as an example for the procedures+tests.

cc @igamigo @bobbinth @Fumuran

Copy link
Contributor

@Fumuran Fumuran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thank you!

I have just one comment about the test, but the assembly code for now looks good for me.

Comment on lines 532 to 544
use.kernel::prologue
use.kernel::note->note_internal
use.kernel::note

begin
exec.prologue::prepare_transaction
exec.note_internal::prepare_note
dropw dropw dropw dropw
exec.note::get_note_script_hash

# truncate the stack
swapw dropw
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You said that you used the test for the get_serial_number as a reference, but it turned out that this test has some flaws in it. Your test (and get_serial_number one) could be rewritten like so:

    use.kernel::prologue
    use.miden::note
    begin
        exec.prologue::prepare_transaction
        exec.note::get_note_script_hash

        # truncate the stack
        swapw dropw
    end

So there are three comments:

  1. As far as I know, we should avoid using procedures from the kernel lib directly, so in that case we should use the get_note_script_hash procedure from the miden lib, and not from the kernel.
  2. We don't need to import the same module with different names. Probably at first there were both miden::note and kernel::note modules, that's why we wanted to have kernel::note as an internal one, but for now it is unnecessary.
  3. We don't need to call note::prepare_note procedure — it just moves some data on the stack, without updating any internal variables. We don't use this data, so this execution could be safely removed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the explanation! I fixed this test and the serial number one.

@tomyrd tomyrd force-pushed the tomyrd-get-script-hash branch from c4b431e to 8c94107 Compare November 29, 2024 20:52
@tomyrd tomyrd marked this pull request as ready for review November 29, 2024 20:56
Copy link
Contributor

@Fumuran Fumuran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

I left just one comment inline.

Comment on lines 543 to 544
let script_hash: Word = tx_context.input_notes().get_note(0).note().script().hash().into();
assert_eq!(process.stack.get_word(0), script_hash);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can avoid importing the Word here by calling the .as_elements() on the RpoDigest returned from the .hash():

let script_hash = tx_context.input_notes().get_note(0).note().script().hash();
assert_eq!(process.stack.get_word(0), script_hash.as_elements());

CHANGELOG.md Outdated
@@ -10,6 +10,7 @@
- [BREAKING] Add error messages to errors and implement `core::error::Error` (#974).
- Implemented new `digest_from_hex!` macro (#984).
- Added Format Guidebook to the `miden-lib` crate (#987).
- Added `miden::note::get_script_hash` procedure (#995).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should note this change as breaking, since it changes the offsets of the kernel procedures.

Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thank you!

@tomyrd tomyrd merged commit f7421e6 into next Dec 2, 2024
9 checks passed
@tomyrd tomyrd deleted the tomyrd-get-script-hash branch December 2, 2024 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants