Skip to content

feat: add note attachments getter APIs#2849

Open
PhilippGackstatter wants to merge 28 commits intopgackst-multiple-note-attachmentsfrom
pgackst-multiple-note-attachments-getters
Open

feat: add note attachments getter APIs#2849
PhilippGackstatter wants to merge 28 commits intopgackst-multiple-note-attachmentsfrom
pgackst-multiple-note-attachments-getters

Conversation

@PhilippGackstatter
Copy link
Copy Markdown
Contributor

This PR adds accessor APIs for multiple attachments.

  • Remove attachment return parameter from get_metadata kernel APIs.
  • Add get_attachments_commitment kernel APIs - this returns the commitment over all attachments of a note.
  • Add various lower and higher-level procedures for working with this API more conveniently (unhashing attachments into its attachment commitments, and unhashing an individual attachment commitment into its data)
    • find_attachment is the most convenient one and should generally be used. Example usage in network_account_target::active_account_matches_target_account.
  • As long as we have no kernel-level support for specifying that a certain attachment scheme can only be added once per note, we need to pick a canonical network account target attachment from the list of attachments, and for now this is the first one.
  • This PR does not yet address many of the previous PR's comments (docs, memory layout, renaming, max attachment elements per note validation, ...). There will be another PR that deals with these follow-ups.

part of #2555

@mmagician mmagician added the pr-from-maintainers PRs that come from internal contributors or integration partners. They should be given priority label Apr 30, 2026
…s' into pgackst-multiple-note-attachments-getters
Copy link
Copy Markdown
Collaborator

@mmagician mmagician left a comment

Choose a reason for hiding this comment

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

As long as we have no kernel-level support for specifying that a certain attachment scheme can only be added once per note, we need to pick a canonical network account target attachment from the list of attachments, and for now this is the first one.

Why would it be desirable to restrict that a note can only carry one attachment of a particular scheme? If this is for the NTX builder to pick up "network notes", I think the builder could still scan all attachments (since there's only four I think that's ok?)

Comment thread crates/miden-protocol/asm/kernels/transaction/api.masm
Comment thread crates/miden-protocol/asm/kernels/transaction/api.masm
Comment thread crates/miden-protocol/asm/protocol/active_note.masm Outdated
Comment thread crates/miden-protocol/asm/protocol/active_note.masm Outdated
Comment thread crates/miden-protocol/asm/protocol/active_note.masm Outdated
Comment thread crates/miden-protocol/asm/protocol/input_note.masm
Copy link
Copy Markdown
Collaborator

@mmagician mmagician left a comment

Choose a reason for hiding this comment

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

LGTM!
There are a few comments still but I'm approving already, assuming that the following get resolved:

  • the test for "retrieves the correct attachment data" should verify the actual data is correct (also maybe at least one of the attachments should be >1 word?)
  • naming of write_attachments_to_memory procedure

Comment thread crates/miden-protocol/asm/protocol/note.masm Outdated
Comment thread crates/miden-protocol/asm/protocol/note.masm Outdated
Comment thread crates/miden-protocol/asm/protocol/note.masm Outdated
Comment thread crates/miden-protocol/asm/protocol/input_note.masm Outdated
Comment thread crates/miden-testing/src/kernel_tests/tx/test_output_note.rs
Comment thread crates/miden-testing/src/kernel_tests/tx/test_output_note.rs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-from-maintainers PRs that come from internal contributors or integration partners. They should be given priority

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants