refactor(note): collapse NoteMetadataHeader into NoteMetadata#2876
Draft
mmagician wants to merge 11 commits intopgackst-multiple-note-attachments-followupfrom
Draft
refactor(note): collapse NoteMetadataHeader into NoteMetadata#2876mmagician wants to merge 11 commits intopgackst-multiple-note-attachments-followupfrom
NoteMetadataHeader into NoteMetadata#2876mmagician wants to merge 11 commits intopgackst-multiple-note-attachments-followupfrom
Conversation
Issue #2874 noted that, after PR #2795, `NoteMetadataHeader` contains more data than `NoteMetadata`, which inverts the usual meaning of "header" and made the type name awkward. Rather than rename, this commit merges the two types so a single `NoteMetadata` carries the user-facing fields (sender, note type, tag) together with the per-attachment headers and attachments commitment that make up the protocol-level metadata. `NoteMetadataHeader` had no external usages as a Rust type beyond ctor and accessor sites, so the merge is mostly mechanical: - `NoteHeader` now holds `NoteMetadata` directly; `metadata_header()` / `into_metadata_header()` accessors are gone, replaced by `metadata()` / `into_metadata()`. - `Note::with_attachments` and `PartialNote::new` now build the metadata via the new `NoteMetadata::with_attachments(&NoteAttachments)` builder. - `compute_note_commitment` takes `&NoteMetadata`. Wire format keeps the same kernel-facing metadata word encoding. To avoid duplicating attachment headers + commitment on the wire (since they are derivable from the `NoteAttachments` payload that follows), `Note` and `PartialNote` use small `pub(super)` `write_core` / `read_core` helpers on `NoteMetadata` that handle just the 3 core fields. Standalone `NoteMetadata` and `NoteHeader` serialization still write the full struct (matching what the old `NoteMetadataHeader` serialized). `TryFrom<Word>` was already removed in #2795. Closes #2874. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PhilippGackstatter
changed the base branch from
pgackst-multiple-note-attachments-getters
to
pgackst-multiple-note-attachments-followup
PhilippGackstatter
force-pushed
the
mmagician-claude/collapse-metadata-header
branch
from
c150ec7 to
bd5cb52
Compare
Contributor
|
Rebased on top of #2871. |
Add a `bon`-generated builder on `Note` so users can construct notes without materializing a `NoteMetadata` of their own. Required fields: sender + recipient. Defaults: assets, attachments, note_tag, note_type. This commit is purely additive: existing `Note::new`, `Note::with_attachments`, and `NoteMetadata::new` / `with_tag` are untouched. Subsequent commits will migrate callers to the builder and tighten the constructor surface. Also drops a redundant `.clone()` on `NoteMetadata` that clippy flagged once the type became `Copy` in the parent commit. Refs #2874. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…d Note::with_attachments All call sites of `Note::new(assets, metadata, recipient)` and `Note::with_attachments(assets, metadata, recipient, attachments)` now go through `Note::builder()`. The legacy ctors are deleted; the builder is the only public way to construct a `Note`. Migrated ~45 sites across miden-protocol, miden-tx, miden-standards, miden-agglayer, and miden-testing. The builder body is inlined (no longer delegates through `Self::with_attachments`) and uses bon's special-case for `pub fn new` so the public surface reads as `Note::builder()...build()`. The testing helper `miden_standards::testing::note::NoteBuilder` keeps its name in this commit; the `NoteBuilder` -> `TestNoteBuilder` rename is a follow-up commit. Refs #2874. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The test helper in `miden-standards::testing::note` was named `NoteBuilder`, which now collides conceptually with `Note::builder()` (the public, production-grade builder added in the previous commit). Rename to `TestNoteBuilder` to make the testing-only character explicit. Pure mechanical rename. No behavioral change. Refs #2874. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…remains After this commit, `NoteMetadata` has a single public constructor: `NoteMetadata::from_parts(sender, note_type, tag, attachment_headers, attachments_commitment)`. The builder helpers (`new`, `with_tag`, `with_attachments`, `set_tag`) are removed so a `NoteMetadata` always represents the final, complete metadata of some note rather than an in-progress object that could be mutated later. Production paths that previously assembled metadata in stages: - `Note::builder()` body now calls `from_parts` directly with derived attachment headers and commitment. - `PartialNote::new` rebuilds the metadata via `from_parts`, taking sender/note_type/tag from the caller's metadata and recomputing attachment fields from the supplied `NoteAttachments`. - `RawOutputNote::into_output_note` for private notes likewise rebuilds. - Encoding tests construct `NoteMetadata::from_parts(...)` directly. Refs #2874. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`NoteType` derives `#[default]` on `Private`, so the builder field defaults to `NoteType::Private` already. Drop the explicit calls at the two `Note::builder()` sites that were specifying it. (The remaining `.note_type(NoteType::Private)` sites in `miden-testing` are on `TestNoteBuilder`, which has its own `Public` default and so still needs the explicit call.)
Comment on lines
+732
to
+738
| Ok(NoteMetadata::from_parts( | ||
| sender, | ||
| note_type, | ||
| tag, | ||
| [NoteAttachmentHeader::absent(); NoteAttachments::MAX_COUNT], | ||
| Word::empty(), | ||
| )) |
Contributor
There was a problem hiding this comment.
This is one of the less nice areas with the new NoteMetadata approach. During tx execution, the attachment schemes in the NoteMetadata field in the OutputNoteBuilder go out of sync with what the tx kernel stores.
In OutputNoteBuilder::build we ignore the attachment info in the metadata anyway, so I would refactor NoteBeforeCreated and OutputNoteBuilder to contain the individual metadata fields rather than the whole type, so we don't have stale attachment info in the metadata. So:
NoteBeforeCreated {
/// The note index extracted from the stack.
note_idx: usize,
/// The sender of the note extracted from the stack.
sender: AccountId,
/// The type of the note extracted from the stack.
note_type: NoteType,
/// The tag of the note extracted from the stack.
note_tag: NoteTag,
/// The recipient data extracted from the advice inputs.
recipient_data: RecipientData,
},and
pub struct OutputNoteBuilder {
sender: AccountId,
note_type: NoteType,
note_tag: NoteTag,
assets: Vec<Asset>,
attachments: Vec<NoteAttachment>,
recipient_digest: Word,
recipient: Option<NoteRecipient>,
}
Collaborator
Author
There was a problem hiding this comment.
Thanks, good catch. It's implemented now in refactor(miden-tx): replace NoteMetadata with sender/note_type/note_t…, although it did unfortunately get way more verbose than I had hoped for.
Contributor
There was a problem hiding this comment.
Mh, yeah that is quite verbose. Looks like even with removing the NoteMetadata construction from the user, there is still value for the protocol crates in having a type that wraps sender, note type and tag.
Maybe we can re-introduce a type that wraps those called NoteAttributes for protocol-internal use (it would still have to be pub since we use it across crates).
I again thought about inlining the metadata header into NoteHeader (changing the constructor tofn new(note_id: NoteId, metadata: NoteMetadata, attachments: &NoteAttachments), and while this works, we'd still need to expose a lower-level constructor as well for usages such as here. It would either have to take what is currently NoteMetadataHeader, or take its raw parts attachment_headers: [NoteAttachmentHeader; NoteAttachments::MAX_COUNT] and attachments_commitment: Word.
So, it seems there is value in having a type for both note metadata and the "header", so a simpler approach is to rename NoteMetadataHeader to something else, maybe NoteMetadataExtension and call it a day?
Collaborator
Author
There was a problem hiding this comment.
I would rather trade a leaner object surface (single metadata struct) for slightly more verbose code, actually. Mostly I worry that we already a lot of similar concepts (headers, attachment & their commitments, metadata) to wrap one's head around, and if we could simplify the interface that would be a net win.
Though your proposal to simply rename and close this PR also sounds pretty good 👍
Contributor
There was a problem hiding this comment.
Though your proposal to simply rename and close this PR also sounds pretty good 👍
I think I'd do that and keep the two distinct types. NoteMetadata is still the main user-facing one, so there is no additional concept they have to deal with if we keep the "header" type (whatever we name it).
…ag in OutputNoteBuilder and NoteBeforeCreated `NoteMetadata` is meant to represent the *complete* metadata of a real note, including attachment headers and commitment. The output-note construction path in `miden-tx` was sneaking around with stub `NoteMetadata` values built via `from_parts(.., [absent; 4], Word::empty())`, then ignoring those stubbed-out fields and recomputing from the real attachments at the very end. That made `NoteMetadata` dishonest in transit. This commit pulls the three user-facing fields (sender, note_type, note_tag) into `OutputNoteBuilder` and the `NoteBeforeCreated` event variant directly, so a `NoteMetadata` is only constructed when the real attachment data is in hand. Specifically: - `OutputNoteBuilder` stores `sender`, `note_type`, `note_tag` instead of `metadata`. `from_recipient_digest` / `from_recipient` now take the three fields. `build()` constructs the final `NoteMetadata` via `Note::builder()` (full notes) or `from_parts` (partial notes, feeding into `PartialNote::new`) only after attachments are resolved. - `TransactionEvent::NoteBeforeCreated` carries the three fields rather than a stub `NoteMetadata`. - `extract_note_metadata` is replaced by `decode_note_type_and_tag` which returns `(NoteType, NoteTag)` directly; the synthetic `NoteMetadata::from_parts(.., [absent;4], Word::empty())` call is gone. - `BaseHost::output_note_from_recipient*` and `on_note_script_requested` (executor side) take the three fields. - `TransactionKernelError::PublicNoteMissingDetails` becomes a struct variant with the three fields plus `recipient_digest`. No protocol-crate or kernel changes. Suggested by Philipp in #2876 (comment). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`RawOutputNote::into_output_note`'s private-note path was rebuilding the metadata via `NoteMetadata::from_parts(.., attachments.to_headers(), attachments.commitment())`. Since the source `Note` came from `Note::builder()`, those fields already match the attachments — the rebuild is a no-op. Just reuse the existing metadata.
…allable fn
The `#[bon::bon] impl Note { #[builder] pub fn new(...) }` block uses a
`pub fn new` signature that bon hides behind `Note::builder()`. A reader
skimming the file might expect `Note::new(...)` to be callable. Make the
doc explicit so it's clear at a glance.
…etry Standalone `NoteMetadata` serialization writes the full struct, but `Note` / `PartialNote` use `write_core` / `read_core` to write only the 3 user-facing fields (since they carry the full `NoteAttachments` blob separately). Add a comment so a future maintainer doesn't try to "fix" the asymmetry by routing all callers through one or the other.
…for #2876 Adds a CHANGELOG entry for the Rust-API surface changes in this PR: the new `Note::builder()`, the removed `Note::new`/`Note::with_attachments` and `NoteMetadata::new`/`with_tag`/`with_attachments`/`set_tag`, the `NoteMetadataHeader` collapse, the `NoteBuilder` -> `TestNoteBuilder` rename, and the `OutputNoteBuilder` / `NoteBeforeCreated` / `PublicNoteMissingDetails` shape changes. Downstream consumers (client, node) will need to adopt these.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Testing out this concept
Closes #2874