feat: add output_not_set_attachment kernel API
#2252
Conversation
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left some comments inline - most a pretty minor.
Also, we should update the kernel API docs to add the new public procedures to the Output Notes section.
| let note_idx = u32::try_from(note_ptr) | ||
| .map_err(|_| TransactionKernelError::other("failed to convert note_ptr to u32")) | ||
| .and_then(|note_ptr| { | ||
| note_ptr | ||
| .checked_sub(OUTPUT_NOTE_SECTION_OFFSET) | ||
| .ok_or_else(|| { | ||
| TransactionKernelError::other("failed to calculate note_idx from note_ptr") | ||
| }) | ||
| .map(|note_ptr| note_ptr / NOTE_MEM_SIZE) | ||
| })?; |
There was a problem hiding this comment.
nit: maybe this should be a helper function?
| const ATTACHMENT_CONTENT_TYPE_NONE=0 | ||
| const ATTACHMENT_CONTENT_TYPE_RAW=1 | ||
| const ATTACHMENT_CONTENT_TYPE_COMMITMENT=2 |
There was a problem hiding this comment.
Should we make these public?
| #! - the attachment_type does not fit into a u32. | ||
| #! | ||
| #! Invocation: exec | ||
| pub proc set_none_attachment |
There was a problem hiding this comment.
nit (feel free to ignore): maybe this could be set_attachment_none or set_attachment_to_none?
Would also be relevant to set_attachment_raw and set_attachment_commitment.
| #! Inputs: [note_idx, attachment_type] | ||
| #! Outputs: [] |
There was a problem hiding this comment.
Why do we need to pass in the attachment_type here? Shouldn't none imply untyped?
| # Constants for different note types | ||
| const PUBLIC_NOTE=1 # 0b01 | ||
| const PRIVATE_NOTE=2 # 0b10 | ||
| const ENCRYPTED_NOTE=3 # 0b11 |
There was a problem hiding this comment.
We no longer need ENCRYPTED_NOTE, right? Is this something to be removed in a subsequent PR?
| # Constants for note attachment content types | ||
| const ATTACHMENT_CONTENT_TYPE_NONE=0 | ||
| const ATTACHMENT_CONTENT_TYPE_RAW=1 | ||
| const ATTACHMENT_CONTENT_TYPE_COMMITMENT=2 |
There was a problem hiding this comment.
We have these defined in a couple of places. Now that we can export/import constants, I wonder if there is a good central place we could put them in.
| #! Panics if: | ||
| #! - any of the attachment types does not fit into a u32. | ||
| #! - the attachment content type is an unknown variant. | ||
| proc validate_attachment_type_info |
There was a problem hiding this comment.
Should we also check that if attachment_type is none, attachment_content_type should be untyped?
| #! | ||
| #! Panics if: | ||
| #! - the content type is None and the ATTACHMENT is not an empty word. | ||
| proc validate_attachment |
There was a problem hiding this comment.
Why do we split attachment validation into two procedures (this one and validate_attachment_type_info)? We could probably have just one procedure - i.e., something like:
#! Inputs: [attachment_content_type, attachment_type, ATTACHMENT]
#! Outputs: []
proc validate_attachment
...
end
| dup.1 exec.memory::get_output_note_metadata_header | ||
| # => [METADATA_HEADER, new_attachment_type_info, note_ptr] | ||
| # => [[attachment_type_info, tag, sender_id_prefix, sender_id_suffix_and_note_type], new_attachment_type_info, note_ptr] | ||
|
|
||
| swap.4 | ||
| # => [[new_attachment_type_info, tag, sender_id_prefix, sender_id_suffix_and_note_type], attachment_type_info, note_ptr] | ||
| # => [METADATA_HEADER, attachment_type_info, note_ptr] | ||
|
|
||
| movup.5 exec.memory::set_output_note_metadata_header | ||
| # => [METADATA_HEADER, attachment_type_info] |
There was a problem hiding this comment.
nit (feel free to ignore): it may be easier to overwrite just the specific element in the metadata header rather than read the full header, update one element, and then save the full header back.
Changes
output_note_set_attachmentto overwrite the attachment of an output note.set_attachmentin the tx host to update the note with the new attachment.miden::protocol.Choices
miden::protocol, i.e.set_none_attachment,set_raw_attachment,set_commitment_attachment. The main use of these is to make it less confusing for users, since a more genericset_attachmentAPI would require setting both attachment content type and attachment type, which are probably easily confused. We can still easily exposeset_attachmentif there is a need.set_none_attachmentdoesn't read very nice, maybeset_empty_attachmentwould be better (which implies a more comprehensive rename). Another alternative could beset_default_attachment.$kernel::output_note::validate_attachmentdoes not validate that the elements of aNoteAttachmentContentType::Commitmentare in the advice provider. I think we could add this, but I'm not yet sure if it's strictly necessary. Since it isn't otherwise accessed in the kernel, the kernel doesn't necessarily care that the preimage is actually present. It would be easy to add this in a separate PR, too.Follow-Ups
output_note::createand replaceauxwith attachment in tests where it makes sense.set_attachmentcall returns the expected value.part of #2109