Skip to content

feat(standards): add P2idNote bon builder, remove create() factory#3

Open
VAIBHAVJINDAL3012 wants to merge 1 commit into
nextfrom
p2id-note-builder
Open

feat(standards): add P2idNote bon builder, remove create() factory#3
VAIBHAVJINDAL3012 wants to merge 1 commit into
nextfrom
p2id-note-builder

Conversation

@VAIBHAVJINDAL3012

@VAIBHAVJINDAL3012 VAIBHAVJINDAL3012 commented May 24, 2026

Copy link
Copy Markdown

Closes 0xMiden#2283.

Summary by CodeRabbit

Release Notes

  • Breaking Changes

    • Replaced P2idNote::create factory method with a builder-based API (P2idNote::builder()) for more flexible note construction.
  • New Features

    • Added fluent builder methods for P2ID notes: .asset(), .attachment(), and .generate_serial_number().
    • Exposed P2ID_SCRIPT_ROOT constant for public access to the P2ID script root hash.

Review Change Stack

@coderabbitai

coderabbitai Bot commented May 24, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

This PR refactors P2idNote from a unit struct with a create() factory method into a full struct with concrete fields and a #[bon]-based builder pattern. The new API exposes a SCRIPT_ROOT constant, provides accessor methods and builder extensions (.asset(), .attachment(), .generate_serial_number()), and implements bidirectional conversion traits (From<P2idNote> and TryFrom<&Note>). All dependent test code is migrated to the new builder API.

Changes

P2idNote Builder Refactoring

Layer / File(s) Summary
P2idNote struct, SCRIPT_ROOT, and builder constructor
crates/miden-standards/src/note/p2id.rs
P2idNote transitions from unit placeholder to a concrete struct with sender, storage, serial_number, note_type, assets, and attachments fields. A public SCRIPT_ROOT lazy-locked constant exposes the compiled P2ID script root. The #[bon]-backed new() constructor and public accessor methods (sender(), target(), script(), script_root(), etc.) are introduced.
Builder extension methods and conversion traits
crates/miden-standards/src/note/p2id.rs
Builder extension methods .asset(), .attachment(), and .generate_serial_number() enable fluent note construction. From<P2idNote> for Note provides infallible conversion; TryFrom<&Note> for P2idNote reconstructs P2idNote from a protocol note while validating script-root matching.
P2idNote unit tests
crates/miden-standards/src/note/p2id.rs
Test module imports expand to include asset and RNG types. New builder-focused test cases validate minimal construction, asset/attachment accumulation, duplicate-faucet rejection, RNG-based serial generation, SCRIPT_ROOT consistency, and round-trip conversions via From/TryFrom.
Public SCRIPT_ROOT re-export
crates/miden-standards/src/note/mod.rs
The p2id::SCRIPT_ROOT is re-exported as P2ID_SCRIPT_ROOT to expose the P2ID script root to external crate users.
Changelog documentation
CHANGELOG.md
A new v0.16.0 (TBD) entry documents the breaking change: P2idNote::create is replaced by the #[bon] builder with new builder methods and SCRIPT_ROOT static, with conversion via From.
Kernel test migrations
crates/miden-testing/src/kernel_tests/block/proposed_block_errors.rs, proven_block_success.rs, crates/miden-testing/src/kernel_tests/tx/test_account_interface.rs, test_output_note.rs, test_tx.rs
Kernel tests replace P2idNote::create(...) calls with P2idNote::builder() chains, explicitly configuring sender/target/note_type/assets, generating serial numbers via builder RNG, and converting results to Note. Imports are adjusted to include Note type.
Mock chain and auth test migrations
crates/miden-testing/src/mock_chain/chain_builder.rs, crates/miden-testing/tests/auth/hybrid_multisig.rs, multisig.rs, multisig_smart.rs
MockChainBuilder and auth test modules switch from P2idNote::create() to builder-based construction, iterating over asset slices, explicitly generating serial numbers via RandomCoin, and converting built notes to Note type. Import statements add Note alongside NoteType.
Script test migrations
crates/miden-testing/tests/scripts/p2id.rs, send_note.rs
Script tests migrate note construction from P2idNote::create() to fluent builder chains, explicitly adding assets one-by-one, generating serial numbers, and appending attachments before building and converting to Note. Imports replace NoteAttachments with Note.

🎯 3 (Moderate) | ⏱️ ~20 minutes

🐰 A struct once plain, now a builder grand,
With SCRIPT_ROOT shining o'er the land!
From create() calls to chains so bright,
All tests now fluent—what a sight!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the primary changes: introducing a bon-based builder pattern for P2idNote and removing the create() factory method.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch p2id-note-builder

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@CHANGELOG.md`:
- Line 7: Changelog line shows an extra period in the example chain for the new
P2idNote builder; update the example under P2idNote::builder() so the call chain
uses a single dot before build (i.e., change the `..build()?` typo to a
single-dot `build()?`), ensuring the example with sender(..).target(..).build()?
and conversion via Note::from(_) is valid and copy-paste safe.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: a6367466-1eca-4c2e-b413-1e671a22b63d

📥 Commits

Reviewing files that changed from the base of the PR and between 2ef8056 and 13abc41.

📒 Files selected for processing (14)
  • CHANGELOG.md
  • crates/miden-standards/src/note/mod.rs
  • crates/miden-standards/src/note/p2id.rs
  • crates/miden-testing/src/kernel_tests/block/proposed_block_errors.rs
  • crates/miden-testing/src/kernel_tests/block/proven_block_success.rs
  • crates/miden-testing/src/kernel_tests/tx/test_account_interface.rs
  • crates/miden-testing/src/kernel_tests/tx/test_output_note.rs
  • crates/miden-testing/src/kernel_tests/tx/test_tx.rs
  • crates/miden-testing/src/mock_chain/chain_builder.rs
  • crates/miden-testing/tests/auth/hybrid_multisig.rs
  • crates/miden-testing/tests/auth/multisig.rs
  • crates/miden-testing/tests/auth/multisig_smart.rs
  • crates/miden-testing/tests/scripts/p2id.rs
  • crates/miden-testing/tests/scripts/send_note.rs

Comment thread CHANGELOG.md

### Changes

- [BREAKING] Replaced `P2idNote::create` factory with a `bon`-based typestate builder. Construct via `P2idNote::builder().sender(..).target(..)..build()?` and convert with `Note::from(_)`. Added `.asset()`, `.attachment()`, `.generate_serial_number()` builder methods and a `SCRIPT_ROOT` static accessor ([#2283](https://github.com/0xMiden/protocol/issues/2283)).

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Fix invalid builder-chain example in changelog.

Line 7 shows ...target(..)..build()?, which has an extra . and presents an invalid call chain. Please change it to ...target(..).build()? so migration instructions are copy-paste safe.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@CHANGELOG.md` at line 7, Changelog line shows an extra period in the example
chain for the new P2idNote builder; update the example under P2idNote::builder()
so the call chain uses a single dot before build (i.e., change the `..build()?`
typo to a single-dot `build()?`), ensuring the example with
sender(..).target(..).build()? and conversion via Note::from(_) is valid and
copy-paste safe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add Rust types with builder API for standard notes

1 participant