fix: propagate P2ID note type so proposals honor private notes (#322)#338
Conversation
buildP2idTransactionRequest (TS) and build_p2id_transaction_request (Rust)
hardcoded NoteType.Public, so a P2ID proposal always created a public note
regardless of what the user selected.
- TS client: buildP2idTransactionRequest accepts options.noteType;
createP2idProposal takes { noteType } and records it in proposal
metadata; the rebuild-from-metadata path parses it back so cosigners
verify and execute the same note visibility.
- Rust client: build_p2id_transaction_request takes NoteType;
TransactionType::P2ID carries note_type (transfer() still defaults to
public, transfer_with_note_type() for explicit choice); ProposalMetadata,
ProposalMetadataPayload, and ExportedMetadata round-trip it.
- Wire format: metadata.note_type is "public" | "private"; it is omitted
for public notes so pre-existing proposals keep their exact wire shape,
and absent still means public. Unknown values are rejected at parse time
instead of being silently rebuilt as a public note that could never match
the signed tx_summary commitment.
- Server + operator client: mirror note_type in the delta-summary proposal
metadata; OpenAPI specs regenerated.
- Examples: demo CLI prompts for note visibility; browser example wrappers
and the smoke harness pass it through.
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughP2ID proposals now support public or private note visibility across Rust and TypeScript clients. The selected type flows through transaction construction, proposal metadata, exports, server conversions, OpenAPI schemas, examples, and tests, while omitted metadata continues to represent public notes. ChangesP2ID proposal contracts and Rust transaction flow
Wire and integration updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Multisig
participant P2IDBuilder
participant Guardian
Client->>Multisig: createP2idProposal(noteType)
Multisig->>P2IDBuilder: build request with noteType
P2IDBuilder->>Guardian: submit metadata.note_type
Guardian-->>Multisig: return proposal metadata
Multisig->>P2IDBuilder: parse metadata.note_type on rebuild
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/miden-multisig-client/src/multisig.test.ts (1)
1334-1334: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNon-null assertion instead of a guard.
mock.calls.at(-1)!asserts non-null whereat()can returnundefined.♻️ Suggested fix
- const lastCall = vi.mocked(buildP2idTransactionRequest).mock.calls.at(-1)!; + const lastCall = vi.mocked(buildP2idTransactionRequest).mock.calls.at(-1); + expect(lastCall).toBeDefined(); expect(lastCall[4]).toMatchObject({ noteType: NoteType.Private });(adjust the following line to use
lastCall?.[4]or narrow after thetoBeDefined()check)As per coding guidelines, "Avoid non-null assertions (
!) where a guard/narrowing can be used in TypeScript."🤖 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 `@packages/miden-multisig-client/src/multisig.test.ts` at line 1334, Update the lastCall access in the test around buildP2idTransactionRequest to remove the non-null assertion. Use optional access such as lastCall?.[4], or first assert/narrow lastCall is defined before accessing its elements, while preserving the existing test behavior.Source: Coding guidelines
packages/miden-multisig-client/src/multisig.ts (1)
848-849: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInline comment violates multisig SDK guideline.
The added
// Omitted for public notes so the wire shape matches pre-#322 proposals.is an inline comment in implementation code.♻️ Suggested fix
- // Omitted for public notes so the wire shape matches pre-#322 proposals. - noteType: p2idNoteTypeToMetadata(options.noteType), + noteType: p2idNoteTypeToMetadata(options.noteType),Move the rationale into the JSDoc above
createP2idProposalif it needs to be documented.As per path instructions, "Do not add inline comments (
// ...) in implementation code in multisig SDKs."🤖 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 `@packages/miden-multisig-client/src/multisig.ts` around lines 848 - 849, Remove the inline comment immediately above noteType in createP2idProposal. If the wire-shape rationale must remain documented, move it into the function’s JSDoc above createP2idProposal; otherwise leave the implementation without an inline comment.Source: Path instructions
🤖 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 `@packages/miden-multisig-client/src/proposal/metadata.ts`:
- Line 29: Update the metadata serialization path around the noteType field to
map an explicit public value to undefined, so re-encoding omits public
visibility from the wire format while preserving non-public note types. Add a
regression test covering decode/re-encode canonicalization of public noteType.
---
Nitpick comments:
In `@packages/miden-multisig-client/src/multisig.test.ts`:
- Line 1334: Update the lastCall access in the test around
buildP2idTransactionRequest to remove the non-null assertion. Use optional
access such as lastCall?.[4], or first assert/narrow lastCall is defined before
accessing its elements, while preserving the existing test behavior.
In `@packages/miden-multisig-client/src/multisig.ts`:
- Around line 848-849: Remove the inline comment immediately above noteType in
createP2idProposal. If the wire-shape rationale must remain documented, move it
into the function’s JSDoc above createP2idProposal; otherwise leave the
implementation without an inline comment.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0aa67025-216b-4181-8c8c-27187f1e72eb
📒 Files selected for processing (31)
crates/miden-multisig-client/src/account.rscrates/miden-multisig-client/src/execution.rscrates/miden-multisig-client/src/export.rscrates/miden-multisig-client/src/payload.rscrates/miden-multisig-client/src/proposal.rscrates/miden-multisig-client/src/transaction/builder.rscrates/miden-multisig-client/src/transaction/payment.rscrates/server/src/delta_summary/mod.rsdocs/openapi-client.jsondocs/openapi-dashboard.jsondocs/openapi.jsonexamples/_shared/multisig-browser/src/multisigApi.tsexamples/demo/src/actions/proposal_management.rsexamples/demo/src/state.rsexamples/smoke-web/src/smokeHarness.tsexamples/web/src/lib/multisigApi.tspackages/guardian-client/src/conversion.test.tspackages/guardian-client/src/conversion.tspackages/guardian-client/src/server-types.tspackages/guardian-client/src/types.tspackages/guardian-operator-client/src/http.tspackages/guardian-operator-client/src/types.tspackages/miden-multisig-client/src/index.tspackages/miden-multisig-client/src/multisig.test.tspackages/miden-multisig-client/src/multisig.tspackages/miden-multisig-client/src/proposal/metadata.test.tspackages/miden-multisig-client/src/proposal/metadata.tspackages/miden-multisig-client/src/transaction.tspackages/miden-multisig-client/src/transaction/p2id.test.tspackages/miden-multisig-client/src/transaction/p2id.tspackages/miden-multisig-client/src/types/proposal.ts
| recipientId: metadata.recipientId, | ||
| faucetId: metadata.faucetId, | ||
| amount: metadata.amount, | ||
| noteType: metadata.noteType, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Omit explicit public visibility during serialization.
Line 29 re-emits noteType: 'public' after a decode/re-encode round trip, contrary to the compatibility contract that public visibility is absent on the wire. Canonicalize 'public' to undefined here and add a regression test.
Proposed fix
- noteType: metadata.noteType,
+ noteType: metadata.noteType === 'public' ? undefined : metadata.noteType,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| noteType: metadata.noteType, | |
| noteType: metadata.noteType === 'public' ? undefined : metadata.noteType, |
🤖 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 `@packages/miden-multisig-client/src/proposal/metadata.ts` at line 29, Update
the metadata serialization path around the noteType field to map an explicit
public value to undefined, so re-encoding omits public visibility from the wire
format while preserving non-public note types. Add a regression test covering
decode/re-encode canonicalization of public noteType.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #338 +/- ##
==========================================
+ Coverage 77.79% 77.86% +0.06%
==========================================
Files 167 167
Lines 30810 30958 +148
==========================================
+ Hits 23970 24105 +135
- Misses 6840 6853 +13 Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
zeljkoX
left a comment
zeljkoX
left a comment
There was a problem hiding this comment.
LGTM
I pushed a small change, mostly doc updates made while testing this one.
Review and modify if needed....
I was able to send both note types.
However I was not able to consume private note. Have you tested that? I wonder should we extend this part for clients(if needed) and for testing(demo example)?
There was a problem hiding this comment.
Pull request overview
Fixes issue #322 by propagating P2ID note visibility end-to-end so proposals correctly rebuild/verify/execute with the originally selected note type (public vs private), while preserving backward compatibility by omitting note_type for public notes.
Changes:
- Added optional
note_type/noteTypeto P2ID proposal metadata across server, Rust/TS clients, operator client, and OpenAPI outputs (absent ⇒ public). - Updated TS and Rust P2ID transaction builders to accept and thread an explicit note type; added strict parsing/rejection of unknown values when rebuilding from metadata.
- Updated docs/examples/tests to cover defaults, private threading, wire-shape stability, and round-trip behavior.
Reviewed changes
Copilot reviewed 37 out of 37 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| spec/api.md | Documents the new optional p2id metadata note_type field and its compatibility semantics. |
| packages/miden-multisig-client/src/types/proposal.ts | Adds typed wire representation for P2ID note visibility (`'public' |
| packages/miden-multisig-client/src/transaction/p2id.ts | Threads noteType into note construction; adds parse/serialize helpers and options type. |
| packages/miden-multisig-client/src/transaction/p2id.test.ts | Adds tests for default/public behavior, private threading, and helper behavior. |
| packages/miden-multisig-client/src/transaction.ts | Re-exports new P2ID metadata helpers/types from the transaction entrypoint. |
| packages/miden-multisig-client/src/proposal/metadata.ts | Encodes/decodes noteType for P2ID proposals and rejects unsupported values. |
| packages/miden-multisig-client/src/proposal/metadata.test.ts | Adds codec round-trip, canonicalization, and rejection tests for noteType. |
| packages/miden-multisig-client/src/multisig.ts | Extends createP2idProposal API and ensures rebuild-from-metadata uses the parsed note type. |
| packages/miden-multisig-client/src/multisig.test.ts | Adds end-to-end test asserting private note_type reaches pushed payload and rebuild path. |
| packages/miden-multisig-client/src/index.ts | Exports the new helpers/types and isP2idNoteVisibility from the package root. |
| packages/miden-multisig-client/README.md | Documents noteType usage and warns about private-note out-of-band sharing requirement. |
| packages/guardian-operator-client/src/types.ts | Adds noteType to dashboard proposal metadata typing for P2ID. |
| packages/guardian-operator-client/src/http.ts | Parses note_type from dashboard records into noteType. |
| packages/guardian-client/src/types.ts | Adds noteType to per-account API proposal metadata shape. |
| packages/guardian-client/src/server-types.ts | Adds note_type to server wire metadata type. |
| packages/guardian-client/src/conversion.ts | Maps note_type ⇄ noteType in client/server conversion helpers. |
| packages/guardian-client/src/conversion.test.ts | Adds round-trip test for noteType ⇄ note_type. |
| examples/web/src/lib/multisigApi.ts | Threads optional noteType through the web example P2ID proposal call. |
| examples/smoke-web/src/smokeHarness.ts | Adds noteType input support and maps 'private' to NoteType.Private for smoke flow. |
| examples/smoke-web/src/App.tsx | Documents smoke harness usage for private P2ID proposals. |
| examples/demo/src/state.rs | Stores note_type in the demo CLI’s custom proposal recipe. |
| examples/demo/src/actions/proposal_management.rs | Prompts for note visibility and uses transfer_with_note_type in demo CLI. |
| examples/_shared/multisig-browser/src/multisigApi.ts | Threads optional noteType through shared browser multisig wrapper. |
| docs/openapi.json | Regenerated OpenAPI including note_type in proposal metadata schema. |
| docs/openapi-dashboard.json | Regenerated dashboard OpenAPI including note_type in proposal metadata schema. |
| docs/openapi-client.json | Regenerated client OpenAPI including note_type in proposal metadata schema. |
| docs/MULTISIG_SDK.md | Documents TS and Rust APIs for selecting private note visibility and associated warning. |
| crates/server/src/delta_summary/mod.rs | Extends server delta-summary proposal metadata with optional note_type. |
| crates/miden-multisig-client/src/transaction/payment.rs | Adds note_type parameter to Rust P2ID transaction request builder + tests. |
| crates/miden-multisig-client/src/transaction/builder.rs | Threads note type through proposal building and wire metadata emission (omit when public). |
| crates/miden-multisig-client/src/proposal.rs | Adds note_type to TransactionType::P2ID and parses metadata note type with rejection on unknown. |
| crates/miden-multisig-client/src/payload.rs | Adds optional wire note_type to payload metadata; omits for public; adds tests. |
| crates/miden-multisig-client/src/lib.rs | Re-exports NoteType alongside NoteId for public API ergonomics. |
| crates/miden-multisig-client/src/export.rs | Threads note_type through export/import metadata. |
| crates/miden-multisig-client/src/execution.rs | Ensures execution builds final request using the proposal’s note_type. |
| crates/miden-multisig-client/src/account.rs | Updates tests/usage to account for new P2ID transaction field. |
| crates/miden-multisig-client/README.md | Updates Rust README examples to include explicit NoteType parameter where needed. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@zeljkoX thanks for the doc updates and the On consuming private notes: what you hit is expected with the current SDK surface, not something this PR regresses. A private note only publishes its commitment on chain, so the recipient's Miden client never learns the note's contents via sync — The missing piece is the out-of-band note transfer:
Neither the multisig SDK nor the demo currently wraps export/import, so there's no way to do this without dropping to the raw client. I'd keep this PR scoped to note-type propagation (your docs already flag the out-of-band requirement) and file a follow-up for:
Happy to open that issue if you agree. |
Sounds good. |
github-project-automation
Bot
moved this from Review
to Release Candidate
in OZ Development for Miden
Summary
Fixes #322 — Guardian P2ID proposals always created
NoteType.Publicnotes, ignoring the user's "Private" selection.buildP2idTransactionRequest(TS) andbuild_p2id_transaction_request(Rust) hardcoded the note type, and proposal metadata had no field to carry it. This PR threads the note type end to end through both clients, matching the TS and Rust implementations.Changes
Wire format
metadata.note_typefield:"public"|"private".consume_notesv1 handling in consume_notes proposal verification depends on the caller's local Miden IndexedDB — should it? #229). Absent ⇒ public, so existing proposals verify and execute unchanged.tx_summarycommitment, which would surface as a confusing "metadata does not match tx_summary" error.TS client (
@openzeppelin/miden-multisig-client)buildP2idTransactionRequest(...)acceptsoptions.noteType(defaults toNoteType.Public).createP2idProposal(recipient, faucet, amount, nonce?, { noteType })records the choice in proposal metadata.noteTypeback, so private proposals are rebuilt as private notes.parseP2idNoteType,p2idNoteTypeToMetadata,P2idTransactionOptions,P2idNoteVisibility,isP2idNoteVisibility.Rust client (
miden-multisig-client)build_p2id_transaction_requesttakes aNoteTypeparameter.TransactionType::P2IDgains anote_typefield;TransactionType::transfer()still defaults to public, andtransfer_with_note_type()is the explicit form.ProposalBuilder, execution,ProposalMetadata/ProposalMetadataPayload, and the export/import round-trip.Server + operator client
note_typemirrored in the delta-summaryProposalMetadata(andguardian-operator-client), so dashboards can surface visibility.cargo run --features evm --bin gen-openapi -- docs).Examples
noteTypethrough.Breaking changes
TransactionType::P2ID, or callbuild_p2id_transaction_request/with_payment_metadata, need to add the note-type argument.TransactionType::transfer()is unchanged.Release note
@openzeppelin/guardian-clientmust be published before@openzeppelin/miden-multisig-client, since the multisig client's metadata codec references the new wire field.Testing
note_type, private threading, unknown-value rejection, public-payload wire-shape stability, and a request-level test that note type changes the built transaction.createP2idProposaltest assertingnote_type: "private"reaches the pushed payload and the rebuild path.gen-openapi --checkpass; all three TS package suites pass;tscclean forexamples/webandexamples/smoke-web.Summary by CodeRabbit
New Features
noteTypefield; omitted values continue to default to public.Bug Fixes