P2ID note builder (inicio review)#8
Open
VAIBHAVJINDAL3012 wants to merge 5 commits into
Open
Conversation
Replace the `P2idNote` marker type and its `P2idNote::create` factory with a `P2idNote` struct built via a `bon` typestate builder (`P2idNote::builder()`): - `.sender()`, `.target()`, and a serial number (`.serial_number()` or `.generate_serial_number(rng)`) are required; `.note_type()` defaults to private and attachments are optional. - `.asset()`/`.assets()` and `.attachment()`/`.attachments()` append items. - P2ID notes must now carry at least one asset, enforced in `new()` via the new `NoteError::MissingAsset`. - `From<P2idNote> for Note` converts infallibly. Migrate all `P2idNote::create` call sites to the builder. Incidental 0-asset P2ID notes in kernel tests move to P2ANY (which legitimately carries no assets), preserving the 0-asset coverage. Part of 0xMiden#2283
- Drop the builder usage description from `P2idNote::new` docs; the typestate builder is self-documenting. - Trim redundant tails from the builder extension method docs. - Remove the "Use P2ANY ..." rationalization comments across the kernel tests and the P2ANY reference in the `setup_test` doc comment. - Assert the note type against `NoteType::default()` instead of a hardcoded `NoteType::Private` in the minimal-builder test. - Add a `compile_fail` doctest showing `.serial_number()` and `.generate_serial_number()` are mutually exclusive at compile time.
Per review feedback, remove the `compile_fail` doctest (and its explanation) on `generate_serial_number`; the typestate already enforces that the serial number cannot be set twice.
- Use `NoteError::other` instead of adding a dedicated `MissingAsset` variant to the protocol crate; the "at least one asset" rule is a standards concern. - Remove the hand-rolled `create_p2id_note_exact` helper and inline the builder (with `.serial_number()`) at all 9 call sites. - Drop the low-value `builder_generates_serial_number` and `into_note_preserves_assets` tests; exercise `generate_serial_number` inside `builder_accumulates_assets` instead. - Remove the 5 duplicate CHANGELOG entries the rebase re-inserted.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThe PR replaces the placeholder ChangesP2idNote builder migration
Sequence Diagram(s)sequenceDiagram
participant P2idNoteBuilder
participant P2idNote
participant NoteAssets
participant NoteAttachments
participant BuiltNote
P2idNoteBuilder->>P2idNoteBuilder: set sender, target, assets, attachments, note type, serial number
P2idNoteBuilder->>P2idNote: build()
P2idNote->>NoteAssets: validate assets
P2idNote->>NoteAttachments: validate attachments
P2idNote-->>BuiltNote: convert via From<P2idNote>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Left over from removing `create_p2id_note_exact`; clippy (-D warnings) flagged it.
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
1 participant
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.
Summary
Full P2ID note builder branch (
vaibhav/p2id-note-builder@104ac301), for review in inicio-labs. This mirrors upstream 0xMiden/protocol#3126 and includes the changes addressing PhilippGackstatter's review.Contents
P2idNotestruct +bontypestate builder (P2idNote::builder()); P2ID notes now require at least one asset.NoteError::otherinstead of a dedicatedMissingAssetvariant; removed the hand-rolledcreate_p2id_note_exacthelper (builder inlined at 9 sites); dropped two low-value tests; deduped CHANGELOG.Verified:
make format-check+ scoped test suite, 1193 tests, 0 failures.Summary by CodeRabbit
P2idNotecreation: use the newP2idNotebuilder flow (P2idNote::builder()) and convert the built value into a finalNoteviaNote::from.P2idNotemust include at least one asset (rejects invalid builds).