-
Notifications
You must be signed in to change notification settings - Fork 105
feat: add tx kernel support for NoteAttachment
#2249
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 tx kernel support for NoteAttachment
#2249
Conversation
bobbinth
left a comment
There was a problem hiding this 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 left some comments inline - all of them pretty small.
crates/miden-protocol/asm/kernels/transaction/lib/prologue.masm
Outdated
Show resolved
Hide resolved
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | ||
| pub struct NoteAttachmentType(u32); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After working with attachment_type and attachment_content_type in the subsequent PR, I ended up not liking these very much. I think they are too easily confused and don't capture their underlying concept very well.
- The
attachment_typeis user-defined and expresses the semantic meaning of the attachment, e.g. "represents the network account for which a network note is intended". - The
attachment_content_typeexpresses the protocol level encoding or format of the attachment.- I think a better alternative is
NoteAttachmentRepresentationshortened toattachment_reprin code. I think this better captures that this value is about how the attachment is represented and says nothing about its content. I also likeNoteAttachmentFormatandNoteAttachmentLayoutto describe the "envelope" or representation of the attachment, rather than its content.
- I think a better alternative is
I don't have a great alternative for attachment_type yet. Something like NoteAttachmentPurpose or NoteAttachmentSemanticId sounds directionally right, but isn't fully convincing. NoteAttachmentContentType is actually also a reasonable alternative for NoteAttachmentType, since this actually identifies the type of the content. This is currently a misnomer and so could be repurposed.
Let me know if you have any ideas.
Edit: After thinking about it more, I think I would:
- Rename
NoteAttachmentTypetoNoteAttachmentId, as a generic user-defined ID that identifies the content of an attachment, but not its representation. - Rename
NoteAttachmentContentTypetoNoteAttachmentFormat.
The rationale is that attachment_format is more likely to be interpreted as the shape in which the attachment comes rather than the content, similar to what "file format" describes for files. Additionally, attachment_id is unlikely to be interpreted as the shape of the attachment, but rather what it semantically represents.
"Attachment format" no longer aligns nicely with NoteAttachmentContent, but I think content_type blurs the lines too much and is worth renaming. We could go with NoteAttachmentContentFormat, but I think the content doesn't really make things clearer and attachment_id and attachment_format are nicer together without content.
I'd do this in a separate PR so we can find a consensus first, and avoid having to update a bunch of PRs that build on top of this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, I agree with the sentiment - though, I think my preferences would be slightly different:
NoteAttachmentContentTypeI would rename intoNoteAttachmentKind- i.e., "what kind of an attachment it is".NoteAttachmentTypeI would rename intoNoteAttachmentTemplateorNoteAttachmentScheme, but maybeNoteAttachmentIdcould also work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the input!
NoteAttachmentKind- i.e., "what kind of an attachment it is".
This works for answering the question you mentioned, but could also be mistaken for "what kind of standardized attachment is it?". Though, format could be argued to have the same problem. So, since I can't come up with anything more precise than kind or format, I think we can go with either. I'll use "kind", unless there are other opinions.
For NoteAttachmentType, we could also make things more concrete and use NoteAttachmentStandard. This would work quite well in the context of standardized attachments:
impl NetworkAccountTarget {
/// The standardized type of [`NetworkAccountTarget`] attachments.
pub const ATTACHMENT_STANDARD: NoteAttachmentStandard = NoteAttachmentStandard::new(1u32);
}When there is no established standard, users can either make up their "own standard" or use NoteAttachmentStandard::custom which is the previous NoteAttachmentType::untyped (value 0).
It does make attachment_standard more "opinionated" in the sense that we expect every attachment to specify what standard it follows (or just say "custom"). It matches how we would like attachments to be used and it is harder to confuse with "attachment kind" than some other alternatives. The downside is that it's a bit awkward to use when users don't follow a standard, but maybe that's fine.
Edit: NoteAttachmentStandard doesn't work either. If a user creates a non-standard NoteAttachmentStandard(42) the name automatically suggests it is standardized, but it's not. We do actually need something like "type" that can represent a range of identifiers, and only a subset of those is actually standardized.
In the absence of other opinions, will go with NoteAttachmentKind and NoteAttachmentScheme.
bobbinth
left a comment
There was a problem hiding this 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!
d231bb7 to
f4dc6ef
Compare
* feat: Implement `NoteAttachment` * chore: Precompute `NoteHeader` in `PartialNote` * feat: Remove aux, exec hint from `NoteMetadata`; add attachment * feat: refactor output note memory layout and metadata building * chore: adapt tx event extraction to new output_note::create stack state * chore: prepare `compute_output_notes_commitment` for attachment hashing * chore: Update input notes commitment to include attachment * chore: include attachment in output notes commitment * chore: return attachment and header from `input_note_get_metadata` * chore: return attachment and header from `output_note_get_metadata` * fix: compilation errors in `miden-testing` * chore: regenerate kernel procedure hashes * chore: add changelog * chore: rename attachment_types to attachment_type_info for clarity * chore: Move content to word encoding to method * chore: rename `Raw` -> `Word` and `Commitment` -> `Array` * chore: Rename `NoteAttachmentCommitment` to `*Array`
* feat: Implement `NoteAttachment` * chore: Precompute `NoteHeader` in `PartialNote` * feat: Remove aux, exec hint from `NoteMetadata`; add attachment * feat: refactor output note memory layout and metadata building * chore: adapt tx event extraction to new output_note::create stack state * chore: prepare `compute_output_notes_commitment` for attachment hashing * chore: Update input notes commitment to include attachment * chore: include attachment in output notes commitment * chore: return attachment and header from `input_note_get_metadata` * chore: return attachment and header from `output_note_get_metadata` * fix: compilation errors in `miden-testing` * chore: regenerate kernel procedure hashes * chore: add changelog * chore: rename attachment_types to attachment_type_info for clarity * chore: Move content to word encoding to method * chore: rename `Raw` -> `Word` and `Commitment` -> `Array` * chore: Rename `NoteAttachmentCommitment` to `*Array`
Introduces the
NoteAttachment. Changes in Rust are complete, while tx kernel support is minimal, meaning all output notes are crated with a defaultNoneattachment.This PR should be fine to review both in one go and by commit.
Main Changes
NoteAttachmenttype in Rust.NoteMetadata:auxis replaced by attachmentexecution_hintisn't useful for local notes (see also RefactorauxintoNoteAttachmentand simplifyNoteTag#2109 (comment)), only for network notes. So it will move to the standardized network note attachment type (introduced in a later PR).NoteAttachmentdoesn't have to be part ofNoteMetadata, but since the word-encoded metadata needs to contain the attachment type info, it is convenient.WordtoNoteMetadataconversion path.NoteMetadatacan still be represented as aWord, including the note's metadata as well as attachment type info. This is called the metadata header. I found this new terminology reduces confusion when dealing withNOTE_METADATA_HEADERandNOTE_ATTACHMENTparameters in MASM. Both of them together form the note metadata, and the alternative of calling the header justNOTE_METADATAwould be confusing, imo.NoteMetadataHeadertype, but it is left private because it doesn't have to be public at this time.input_note_get_metadataandoutput_note_get_metadatakernel APIs to return both metadata header and attachment.auxandexecution_hintparameters fromoutput_note_create, but I decided to pull this out into a separate PR, so there is some temporary compatibility logic. So, eventuallyoutput_note::createwill take just[tag, note_type, RECIPIENT].output_note_createbuilds the metadata header.NOTE_METADATAis replaced byNOTE_METADATA_COMMITMENTwhich ishash(NOTE_METADATA_HEADER || NOTE_METADATA_ATTACHMENT).Minor Changes
$kernel::memory::get_input_note_senderbecause it was unused and would have required an update.mem_stream hpermin output notes commitment with manual hashing, since the note metadata commitment must be computed first. We could maybe optimize this later by moving metadata commitment to memory and compute it when sealing a note (would require that all notes must be sealed at some point, which I would prefer anyway if we have sealing).NoteMetadataserialization can no longer reuse the word encoding because it must serialize the full attachment whereas the word encoding only serializes the attachment's commitment.NoteHeaderencodings to felts that don't seem to be needed. These no longer work because metadata cannot be decoded from a word anymore.PartialNoteis changed to contain aNoteHeader, so it can easily return&NoteHeader. Useful forOutputNote::headerwhich would otherwise have to clone headers.Noteconversions toNoteHeaderare removed since it is no longerCopy.Follow-Ups
The immediate follow-up to this PR is:
auxandexecution_hintparams fromoutput_note::create.miden::protocolfor accessing attachment type, content type and attachment value in one go.Later also:
NoteExecutionHinttomiden-standards.part of #2109