refactor(pswap): follow-ups for #3000 (AssetAmount + NonZeroU32, parsing, MASM asserts, file split, zero-amount fix)#4
Open
VAIBHAVJINDAL3012 wants to merge 14 commits into
Open
Conversation
- PswapNoteStorage::requested_asset_amount() returns AssetAmount. - PswapNote::create_args takes AssetAmount on both args and is infallible. - PswapNoteAttachment::new takes depth: NonZeroU32 (new is infallible). - parent_depth returns u32; next_depth() yields NonZeroU32 with overflow check. - payback_note/remainder_note drop runtime depth==0 checks (now type-enforced). - Split fill_sum u64-overflow test to hand-built note_args (create_args now rejects > AssetAmount::MAX client-side; MASM check still exercised). Closes group A of issue 0xMiden#3000.
- Add TryFrom<&NoteAttachment> for PswapNoteAttachment validating scheme, num_words == 1, AssetAmount range, and non-zero u32 depth. - Reroute PswapNote::parent_depth through the typed TryFrom so any malformed attachment degrades to 0 in a single place. - Inline the deleted pswap_output_attachment helper at the two callers using PswapNoteAttachment::new(...).into(); construction stays infallible apart from the AssetAmount::new check that was already there. - Migrate the integration tests' first_attachment_word helper to first_pswap_attachment, exercising the new TryFrom on every output note (9 sites total). Closes group B of issue 0xMiden#3000.
…rrent_depth
- New constant PSWAP_ATTACHMENT_NUM_WORDS = 1 and error code
ERR_PSWAP_ATTACHMENT_WRONG_NUM_WORDS gate the write_attachment_to_memory
result before any loc_load, ruling out multi-word attachments overwriting
adjacent local memory.
- After loc_load.PARENT_ATTACHMENT_DEPTH_OFFSET, split into (lo, hi) and
assertz hi == 0 via ERR_PSWAP_ATTACHMENT_DEPTH_NOT_U32. Prefer manual hi
check over u32assert.err=... so the named error surfaces verbatim at the
test boundary (the VM's u32assert wraps its message at runtime).
- Sharpened the procedure docstring: the initial (depth-0) PSWAP carries no
PSWAP_ATTACHMENT_SCHEME attachment; only payback / remainder notes do.
- Integration tests pswap_assert_attachment_wrong_num_words and
pswap_assert_attachment_depth_not_u32 stamp malformed attachments via
NoteAttachment::{with_words,with_word} and exercise both asserts.
Closes group C of issue 0xMiden#3000.
…tId view
- Split crates/miden-standards/src/note/pswap.rs (1430 lines) into pswap/{mod,
storage,attachment,tests}.rs. mod.rs now hosts the PswapNote struct and the
new OrderId newtype; storage.rs and attachment.rs own their respective types
and impls; tests.rs holds the unit tests. git mv preserves history on the old
pswap.rs -> pswap/mod.rs path.
- Introduce OrderId(Felt) - a transparent newtype that PswapNote::order_id and
PswapNoteAttachment{::new,::order_id} now use. From<Felt>/From<OrderId> keep
the MASM bridge cheap.
- Add PswapNoteStorage::requested_asset_id() returning the existing AssetId
type (suffix, prefix). On-chain layout is unchanged - the typed accessor is
a Rust-side convenience.
- PARENT_ATTACHMENT_DEPTH_OFFSET elevated to pub(super) so the new
attachment.rs TryFrom can consume it without duplicating the literal.
- Re-export OrderId from crates/miden-standards/src/note/mod.rs.
- New unit tests cover OrderId round-trips, the order_id == serial[1]
invariant on PswapNote, and the requested_asset_id() packing.
Closes group D of issue 0xMiden#3000.
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughRefactors PSWAP into typed Rust submodules (OrderId, PswapNoteAttachment, PswapNoteStorage), strengthens MASM attachment validation (single-word + u32-fit), updates PswapNote APIs and execution (payout, payback, remainder), makes StandardNote script lookups fallible, and adds extensive Rust and MASM tests plus callsite/executor adjustments. ChangesPSWAP Note Refactor and Validation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
Comment |
Author
|
@CodeRabbit help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
crates/miden-testing/tests/scripts/pswap.rs (1)
226-230: ⚡ Quick winFeed the parsed attachment through the reconstruction path.
These branches only trust
amount()fromfirst_pswap_attachment;order_idanddepthare re-derived from local state. That weakens the new typed-attachment coverage and can miss a decode/accessor regression while the reconstruction tests still pass. Reusing the parsedPswapNoteAttachmenthere keeps the test end-to-end.♻️ Suggested change
- let payback_attachment = PswapNoteAttachment::new( - AssetAmount::new(fill_amount_from_aux)?, - pswap.order_id(), - NonZeroU32::new(1).unwrap(), - ); - let reconstructed_payback = - pswap.payback_note(output_p2id.metadata().sender(), &payback_attachment)?; + let reconstructed_payback = + pswap.payback_note(output_p2id.metadata().sender(), &on_chain_pswap_att)?; ... - let remainder_attachment = PswapNoteAttachment::new( - AssetAmount::new(amt_payout_from_attachment)?, - pswap.order_id(), - NonZeroU32::new(1).unwrap(), - ); let reconstructed_remainder = pswap.remainder_note( output_remainder.metadata().sender(), - &remainder_attachment, + &remainder_pswap_att, remaining_offered, remaining_requested, )?; ... - let payback_attachment = PswapNoteAttachment::new( - on_chain_payback_att.amount(), - original_pswap.order_id(), - depth, - ); let reconstructed_payback = original_pswap - .payback_note(on_chain_payback.metadata().sender(), &payback_attachment)?; + .payback_note(on_chain_payback.metadata().sender(), &on_chain_payback_att)?; ... - let remainder_attachment = PswapNoteAttachment::new( - on_chain_remainder_att.amount(), - original_pswap.order_id(), - depth, - ); let reconstructed_remainder = original_pswap.remainder_note( on_chain_remainder.metadata().sender(), - &remainder_attachment, + &on_chain_remainder_att, AssetAmount::new(remaining_offered)?, AssetAmount::new(remaining_requested)?, )?;Also applies to: 258-262, 1769-1773, 1788-1792
🤖 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 `@crates/miden-testing/tests/scripts/pswap.rs` around lines 226 - 230, The test is reconstructing order_id and depth from local state instead of reusing the parsed PswapNoteAttachment, which weakens end-to-end coverage; update the payback_attachment creation (and the similar instances at the other locations) to derive the attachment from the parsed first_pswap_attachment (e.g., clone or otherwise reuse first_pswap_attachment and only adjust the amount) rather than calling PswapNoteAttachment::new with pswap.order_id() and NonZeroU32::new(1).unwrap(), so the reconstruction path exercises the parsed attachment's order_id(), depth(), and accessors directly.
🤖 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 `@crates/miden-standards/asm/standards/notes/pswap.masm`:
- Around line 522-528: The current guard (eq.PSWAP_ATTACHMENT_NUM_WORDS /
assert.err=ERR_PSWAP_ATTACHMENT_WRONG_NUM_WORDS) runs after
exec.active_note::write_attachment_to_memory has already copied into the 4-word
`@locals`(4) buffer, so a forged multi-word attachment can clobber locals before
failing; fix by performing the attachment-length check before any write (i.e.,
check num_words == 1 immediately in exec.active_note::write_attachment_to_memory
or its caller) or else allocate/use a scratch region sized for the maximum
allowed attachment length and validate bounds prior to writing; reference
exec.active_note::write_attachment_to_memory, `@locals`(4),
eq.PSWAP_ATTACHMENT_NUM_WORDS and ERR_PSWAP_ATTACHMENT_WRONG_NUM_WORDS when
making the change.
In `@crates/miden-standards/src/note/pswap/attachment.rs`:
- Around line 62-87: The TryFrom<&NoteAttachment> for PswapNoteAttachment
implementation currently ignores the reserved trailing limb (word[3]) and must
reject non-zero values to preserve canonical encoding; update the function (in
the PswapNoteAttachment conversion block) to inspect words[0..] and if word[3]
is not zero return Err(NoteError::other("PSWAP attachment reserved trailing limb
must be zero")) before constructing Self { amount, order_id:
OrderId::from(word[1]), depth }, so malformed attachments that set the reserved
slot are rejected rather than silently accepted.
In `@crates/miden-standards/src/note/pswap/mod.rs`:
- Around line 430-437: The reconstructed child serials are being advanced using
the absolute attachment.depth() (parent_depth) which over-increments when called
on a remainder parent; update the p2id_serial construction (and the analogous
block used later around create_payback_note/create_remainder_pswap_note) to
advance the serial relative to the direct parent by adding ONE to
self.serial_number[3] (instead of adding parent_depth/attachment.depth()), so
use self.serial_number[3] + ONE for the fourth word component when building
p2id_serial.
- Around line 311-324: The match arm that handles a single fill source (the
(Some(asset), None) | (None, Some(asset)) branch) must validate that that asset
matches the requested faucet asset before proceeding to create_payback_note();
currently it bypasses add() and allows minting with a non-requested asset.
Update the code around account_fill_asset and note_fill_asset to compare the
lone asset to self.storage.requested_asset() and return a NoteError (e.g.,
NoteError::other with a clear message) if they differ, otherwise continue using
that asset as payback_asset; keep the existing add() path unchanged for the
(Some, Some) case.
In `@crates/miden-standards/src/note/pswap/tests.rs`:
- Around line 131-134: Update the test for PswapNote::calculate_output_amount to
assert the exact floored result for the non-integer ratio case: replace the
loose assert!("> 0") with an equality check expecting 9 (since 100 * 7 / 73
floors to 9). Locate the call to PswapNote::calculate_output_amount(100, 73, 7)
in the tests and change the assertion to assert_eq!(result, 9) so incorrect
implementations that return any positive value will fail.
---
Nitpick comments:
In `@crates/miden-testing/tests/scripts/pswap.rs`:
- Around line 226-230: The test is reconstructing order_id and depth from local
state instead of reusing the parsed PswapNoteAttachment, which weakens
end-to-end coverage; update the payback_attachment creation (and the similar
instances at the other locations) to derive the attachment from the parsed
first_pswap_attachment (e.g., clone or otherwise reuse first_pswap_attachment
and only adjust the amount) rather than calling PswapNoteAttachment::new with
pswap.order_id() and NonZeroU32::new(1).unwrap(), so the reconstruction path
exercises the parsed attachment's order_id(), depth(), and accessors directly.
🪄 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: c954cdf8-03a9-4262-907f-2849d73494a9
📒 Files selected for processing (8)
crates/miden-standards/asm/standards/notes/pswap.masmcrates/miden-standards/src/note/mod.rscrates/miden-standards/src/note/pswap.rscrates/miden-standards/src/note/pswap/attachment.rscrates/miden-standards/src/note/pswap/mod.rscrates/miden-standards/src/note/pswap/storage.rscrates/miden-standards/src/note/pswap/tests.rscrates/miden-testing/tests/scripts/pswap.rs
💤 Files with no reviewable changes (1)
- crates/miden-standards/src/note/pswap.rs
Comment on lines
522
to
+528
| exec.active_note::write_attachment_to_memory | ||
| # => [num_words] | ||
|
|
||
| drop | ||
| # A PswapAttachment is exactly one word - assert before reading back from @locals(4) to | ||
| # rule out an attacker-crafted multi-word attachment overwriting adjacent local memory. | ||
| eq.PSWAP_ATTACHMENT_NUM_WORDS | ||
| assert.err=ERR_PSWAP_ATTACHMENT_WRONG_NUM_WORDS |
There was a problem hiding this comment.
The attachment-length guard runs after the unsafe write.
write_attachment_to_memory has already copied the payload into a 4-local buffer before num_words == 1 is asserted, so a forged multi-word PSWAP attachment can still clobber adjacent local memory and only then fail. If there is no length-only probe, this needs a scratch region sized for the maximum attachment length or another pre-write bound check; the current check does not provide the protection described in the comment.
🤖 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 `@crates/miden-standards/asm/standards/notes/pswap.masm` around lines 522 -
528, The current guard (eq.PSWAP_ATTACHMENT_NUM_WORDS /
assert.err=ERR_PSWAP_ATTACHMENT_WRONG_NUM_WORDS) runs after
exec.active_note::write_attachment_to_memory has already copied into the 4-word
`@locals`(4) buffer, so a forged multi-word attachment can clobber locals before
failing; fix by performing the attachment-length check before any write (i.e.,
check num_words == 1 immediately in exec.active_note::write_attachment_to_memory
or its caller) or else allocate/use a scratch region sized for the maximum
allowed attachment length and validate bounds prior to writing; reference
exec.active_note::write_attachment_to_memory, `@locals`(4),
eq.PSWAP_ATTACHMENT_NUM_WORDS and ERR_PSWAP_ATTACHMENT_WRONG_NUM_WORDS when
making the change.
VAIBHAVJINDAL3012
pushed a commit
that referenced
this pull request
May 29, 2026
…ction to original Address review feedback on PR #4: - execute(): when only one fill source is provided, the path bypassed FungibleAsset::add, which is the only place a faucet mismatch would have been caught. A caller could mint a payback note whose asset disagrees with the storage's requested_faucet_id. The MASM rejects this on-chain but the Rust API now catches it client-side via a per-source faucet check at the top of execute(). New unit test exercises both single-source arms. - payback_note() and remainder_note() advance the serial using the absolute lineage depth (depth - 1). Calling them on a remainder mid-lineage over-counts that delta and reconstructs the wrong note. Document the precondition (must be called on the original PSWAP) at the top of each doc comment so the contract is loud, without adding a runtime guard that would change the existing contract surface.
| # write the word at PARENT_ATTACHMENT_PTR, read back only the depth at DEPTH_OFFSET. | ||
| const PARENT_ATTACHMENT_PTR = 0 | ||
| const PARENT_ATTACHMENT_DEPTH_OFFSET = 2 | ||
| # PswapAttachment is always exactly one word - see PSWAP_ATTACHMENT_SCHEME above. |
Author
There was a problem hiding this comment.
Remove this see line - see PSWAP_ATTACHMENT_SCHEME above.
|
|
||
| drop | ||
| # A PswapAttachment is exactly one word - assert before reading back from @locals(4) to | ||
| # rule out an attacker-crafted multi-word attachment overwriting adjacent local memory. |
Author
There was a problem hiding this comment.
remove this comment not required.....self understandable
| drop | ||
| # => [hi, parent_depth] | ||
| assertz.err=ERR_PSWAP_ATTACHMENT_DEPTH_NOT_U32 | ||
| # => [parent_depth] |
Author
There was a problem hiding this comment.
Remove this long comment
And see if their direct way to check if value is u32 or not
| let depth = NonZeroU32::new(depth_u32) | ||
| .ok_or_else(|| NoteError::other("PSWAP attachment depth must be non-zero"))?; | ||
|
|
||
| Ok(Self { amount, order_id: OrderId::from(word[1]), depth }) |
Author
There was a problem hiding this comment.
Add the check for word[3] as 0 too.
Comment on lines
+24
to
+25
| /// Slots `[1, 2]` together form a 2-element [`AssetId`] view of the faucet (see | ||
| /// [`Self::requested_asset_id`]). The on-chain layout is unchanged - the typed accessor is a |
Author
There was a problem hiding this comment.
The on-chain layout is unchanged - the typed accessor is a
remvoe this comment...
| self.serial_number[3] + ONE, | ||
| ]); | ||
|
|
||
| let current_depth = self.next_depth()?; |
| consumer_account_id: AccountId, | ||
| remaining_offered_asset: FungibleAsset, | ||
| remaining_requested_asset: FungibleAsset, | ||
| offered_amount_for_fill: u64, |
Author
There was a problem hiding this comment.
It should be AssetAmount,.....not u64
| let recipient = pswap.storage.into_recipient(pswap.serial_number); | ||
|
|
||
| let assets = NoteAssets::new(vec![pswap.offered_asset.into()]) | ||
| .expect("single fungible asset should be valid"); |
Author
There was a problem hiding this comment.
Shouldn't use expect, as it would panic.
| Asset::NonFungible(_) => { | ||
| return Err(NoteError::other("PSWAP note asset must be fungible")); | ||
| }, | ||
| }; |
Author
There was a problem hiding this comment.
note onmly has one asset, so no need to iter it......
| .build(); | ||
| let expected = AssetId::new(faucet_id.suffix(), faucet_id.prefix().as_felt()); | ||
| assert_eq!(storage.requested_asset_id(), expected); | ||
| } |
Author
There was a problem hiding this comment.
check in the test if we can use rtest
VAIBHAVJINDAL3012
pushed a commit
that referenced
this pull request
May 29, 2026
Addresses the 26 inline review comments on #4 in one pass. MASM (pswap.masm): - Drop the redundant 'see PSWAP_ATTACHMENT_SCHEME above' cross-reference and the self-explanatory comment on the num_words == 1 assert. - Shorten the depth-check comment to a one-line note on why u32assert.err is avoided (VM wraps the message at runtime). attachment.rs: - TryFrom now asserts word[3] == 0 (the reserved slot). Matches the encoder's invariant and stops malformed attachments from round-tripping to a different value. New unit test try_from_rejects_non_zero_reserved_slot. storage.rs: - Trim three doc blurbs (the on-chain-layout note, the payback_note_type rationale, the AssetId description) to focus on what the type is, not why. - Replace the .expect on NoteStorage::new with const _: () = assert! + unwrap_or_else(|_| unreachable!()), giving compile-time proof that the serialization path is unreachable on its error branch. mod.rs: - Drop 'lineage' from OrderId / order_id / payback_note / remainder_note docs; drop the network-account paragraphs from PswapNote / attachments() docs; shorten the zero-amount guard comment to one line. - Rename current_depth -> depth in code and docs throughout. - Migrate calculate_output_amount, calculate_offered_for_requested, execute, and create_remainder_pswap_note to take/return AssetAmount end-to-end (callers stop bouncing through u64). - Remove the .iter().flatten() on the 2-element fill array; use a small is_some_and closure instead. - Replace note.assets().iter().next().unwrap() + match Asset variant with note.assets().iter_fungible().next().ok_or_else(...) on the TryFrom<&Note> reconstruction path. - Replace the num_attachments() match + .expect on get(0) with a >1 guard followed by attachments.get(0).cloned() (no panic path). - Drop the now-redundant fill_amount param from create_payback_note; payback_asset.amount() carries it. - Drop the duplicated 'matching MASM add.1' / 'matching MASM movup.3 add.1 movdn.3' inline comments on serial derivations. tests.rs: - Update calculate_output_amount unit test to AssetAmount and assert the exact floor result for the non-integer ratio case (CodeRabbit #5). - Add try_from_rejects_non_zero_reserved_slot covering the new word[3] guard. miden-testing/tests/scripts/pswap.rs: - Update six calculate_offered_for_requested call sites to pass AssetAmount and unwrap the AssetAmount result back to u64 where downstream code needs it for vault/amount arithmetic. Verified: - cargo +nightly fmt --all clean. - cargo clippy -p miden-standards -p miden-testing --all-targets -- -D warnings clean on both crates. - 29 unit tests + 72 integration tests pass.
Without this guard, calling calculate_offered_for_requested on a PswapNote with requested_amount == 0 panics with an unchecked integer division by zero at u128 / 0 in calculate_output_amount. The Rust panic is reachable only through direct API misuse (execute short-circuits via fill > requested), but it is still a Rust panic rather than a NoteError, which is the wrong failure mode for a typed builder API. Fix the root cause by rejecting zero on both sides in PswapNote::build: - offered_amount == 0 means the note pays out nothing on any fill (useless). - requested_amount == 0 is the div-by-zero path. Placing the check in build covers every constructor: the typed builder itself, and TryFrom<&Note> which funnels through it on the reconstruction path. Storage- and attachment-level reconstruction stays permissive because the on-chain MASM's u128::div would have rejected such a note anyway - the new check just moves that rejection to the Rust layer so callers get a typed NoteError instead of a panic. Two new unit tests cover both rejection paths.
…ction to original Address review feedback on PR #4: - execute(): when only one fill source is provided, the path bypassed FungibleAsset::add, which is the only place a faucet mismatch would have been caught. A caller could mint a payback note whose asset disagrees with the storage's requested_faucet_id. The MASM rejects this on-chain but the Rust API now catches it client-side via a per-source faucet check at the top of execute(). New unit test exercises both single-source arms. - payback_note() and remainder_note() advance the serial using the absolute lineage depth (depth - 1). Calling them on a remainder mid-lineage over-counts that delta and reconstructs the wrong note. Document the precondition (must be called on the original PSWAP) at the top of each doc comment so the contract is loud, without adding a runtime guard that would change the existing contract surface.
Re-flows imports, multi-line argument lists, and a few long expressions through cargo +nightly fmt with the repo's rustfmt.toml. No behavior change. Confirmed cargo clippy -p miden-standards --all-targets -D warnings and -p miden-testing --all-targets -D warnings are clean.
Addresses the 26 inline review comments on #4 in one pass. MASM (pswap.masm): - Drop the redundant 'see PSWAP_ATTACHMENT_SCHEME above' cross-reference and the self-explanatory comment on the num_words == 1 assert. - Shorten the depth-check comment to a one-line note on why u32assert.err is avoided (VM wraps the message at runtime). attachment.rs: - TryFrom now asserts word[3] == 0 (the reserved slot). Matches the encoder's invariant and stops malformed attachments from round-tripping to a different value. New unit test try_from_rejects_non_zero_reserved_slot. storage.rs: - Trim three doc blurbs (the on-chain-layout note, the payback_note_type rationale, the AssetId description) to focus on what the type is, not why. - Replace the .expect on NoteStorage::new with const _: () = assert! + unwrap_or_else(|_| unreachable!()), giving compile-time proof that the serialization path is unreachable on its error branch. mod.rs: - Drop 'lineage' from OrderId / order_id / payback_note / remainder_note docs; drop the network-account paragraphs from PswapNote / attachments() docs; shorten the zero-amount guard comment to one line. - Rename current_depth -> depth in code and docs throughout. - Migrate calculate_output_amount, calculate_offered_for_requested, execute, and create_remainder_pswap_note to take/return AssetAmount end-to-end (callers stop bouncing through u64). - Remove the .iter().flatten() on the 2-element fill array; use a small is_some_and closure instead. - Replace note.assets().iter().next().unwrap() + match Asset variant with note.assets().iter_fungible().next().ok_or_else(...) on the TryFrom<&Note> reconstruction path. - Replace the num_attachments() match + .expect on get(0) with a >1 guard followed by attachments.get(0).cloned() (no panic path). - Drop the now-redundant fill_amount param from create_payback_note; payback_asset.amount() carries it. - Drop the duplicated 'matching MASM add.1' / 'matching MASM movup.3 add.1 movdn.3' inline comments on serial derivations. tests.rs: - Update calculate_output_amount unit test to AssetAmount and assert the exact floor result for the non-integer ratio case (CodeRabbit #5). - Add try_from_rejects_non_zero_reserved_slot covering the new word[3] guard. miden-testing/tests/scripts/pswap.rs: - Update six calculate_offered_for_requested call sites to pass AssetAmount and unwrap the AssetAmount result back to u64 where downstream code needs it for vault/amount arithmetic. Verified: - cargo +nightly fmt --all clean. - cargo clippy -p miden-standards -p miden-testing --all-targets -- -D warnings clean on both crates. - 29 unit tests + 72 integration tests pass.
VAIBHAVJINDAL3012
force-pushed
the
vaibhav/pswap-refactor
branch
from
f24004b to
0565c0f
Compare
| loc_load.PARENT_ATTACHMENT_DEPTH_OFFSET | ||
| # => [parent_depth] | ||
|
|
||
| # Manual hi==0 check rather than u32assert.err=... because the VM wraps u32assert's |
Author
There was a problem hiding this comment.
remove this comment....
| # => [parent_depth] | ||
|
|
||
| # Manual hi==0 check rather than u32assert.err=... because the VM wraps u32assert's | ||
| # message at runtime, which would defeat exact-match assertions on the named error. |
Author
There was a problem hiding this comment.
remove this above line
| # message at runtime, which would defeat exact-match assertions on the named error. | ||
| dup u32split drop | ||
| # => [hi, parent_depth] | ||
| assertz.err=ERR_PSWAP_ATTACHMENT_DEPTH_NOT_U32 |
| &self, | ||
| consumer_account_id: AccountId, | ||
| account_fill_asset: Option<FungibleAsset>, | ||
| note_fill_asset: Option<FungibleAsset>, |
Author
There was a problem hiding this comment.
Use Option rahter than asset.....
| }; | ||
| if wrong_faucet(&account_fill_asset) || wrong_faucet(¬e_fill_asset) { | ||
| return Err(NoteError::other("fill asset faucet does not match the requested faucet")); | ||
| } |
Author
There was a problem hiding this comment.
this wrong_Facuet will go once we do the get the asseAmount input
| ))); | ||
| } | ||
|
|
||
| // Mirror the MASM, which computes payouts separately for the account and note halves |
Author
There was a problem hiding this comment.
Dont say Mirror the Masm, it not required..
|
|
||
| // Mirror the MASM, which computes payouts separately for the account and note halves | ||
| // (the account half flows to the consumer, the sum drives the remainder's offered). | ||
| let account_fill_amount = account_fill_asset.map_or(AssetAmount::ZERO, |a| a.amount()); |
Author
There was a problem hiding this comment.
We would direclty get the amount now..
Comment on lines
+572
to
+573
| let product = (offered_total.as_u64() as u128) * (fill_amount.as_u64() as u128); | ||
| let quotient = product / (requested_total.as_u64() as u128); |
Author
There was a problem hiding this comment.
use better vaiable name
VAIBHAVJINDAL3012
left a comment
VAIBHAVJINDAL3012
left a comment
Author
There was a problem hiding this comment.
Please make a change
…Note Mirrors the storage.rs fix from the previous commit. The expect was provably unreachable but still a panic-capable expression. Replace with a compile-time assertion (1 <= NoteAssets::MAX_NUM_ASSETS) plus unwrap_or_else(|_| unreachable!()) so the path is unreachable by construction.
Addresses the second review batch on #4. API: PswapNote::execute now takes Option<AssetAmount> on both fill slots instead of Option<FungibleAsset>. The faucet is implicit (it must be the PSWAP's requested faucet), so passing a typed amount removes the only caller-visible mismatch surface. Callers no longer construct FungibleAsset client-side to invoke execute. Knock-on cleanups: - Drop the wrong-faucet guard added in 9365d38 and its unit test (unreachable now that callers can't pass a mismatched faucet). - Drop the 'Mirror the MASM' comment in execute. - Collapse map_or(AssetAmount::ZERO, |a| a.amount()) to a direct unwrap_or(AssetAmount::ZERO) per the AssetAmount signature. - Rename calculate_output_amount's intermediate to . MASM: drop the 2-line u32assert-rationale comment + blank line above the depth check; add a blank line after the # => [hi, parent_depth] annotation so the assertz lines visually separate. Tests: all 24 .execute() call sites in tests/scripts/pswap.rs and src/note/pswap/tests.rs updated to pass AssetAmount (or Asset::amount()) on the fill slots. 28 unit + 72 integration tests pass; cargo +nightly fmt and cargo clippy -p miden-standards -p miden-testing --all-targets -- -D warnings clean.
The doc still referenced `account_fill_asset` / `note_fill_asset` — both renamed to `account_fill` / `note_fill` when execute() switched to Option<AssetAmount>.
…expect Drop the last .expect on the PSWAP_SCRIPT LazyLock. The init can only fail if the standards library is built without the PSWAP procedure (a build-time invariant), but we now surface the failure as a NoteError so callers control the failure mode rather than getting a panic. Storage of the load result: - LazyLock<Result<NoteScript, String>> (NoteError is not Clone, so the error side is held as a String and re-wrapped into a fresh NoteError on each access via pswap_script_error). API ripple (all build-time-invariant, never expected to fire at runtime): - PswapNote::script() / ::script_root() / ::create_tag() now return Result. - PswapNoteStorage::into_recipient() now returns Result. - From<PswapNote> for Note becomes TryFrom<PswapNote> for Note. - StandardNote::from_script(), ::from_script_root(), ::script(), ::script_root() now return Result (the PSWAP arm propagates; other arms are wrapped in Ok). from_script* now return Result<Option<Self>>. - TryFrom<&Note> for PswapNote uses script_root()? . Call-site updates: - Tests: 24 Note::from(pswap) / pswap.into() sites → Note::try_from(pswap)? . - crates/miden-tx executor + notes_checker: thread the Result through exec_host's StandardNote::from_script_root + .script() path, and treat the build-time failure as 'not a known standard note' in the checker's ordering / consumption-status paths so existing behavior is preserved. - bin/bench-transaction, miden-testing/tests/agglayer/bridge_out, miden-testing/tests/scripts/faucet, miden-testing/src/kernel_tests: minor ? / .unwrap() additions on infallible-by-construction call sites. Verified: cargo +nightly fmt --all clean; cargo clippy on miden-standards, miden-testing, miden-tx --all-targets -- -D warnings clean; 28 unit + 72 integration tests pass.
- storage.rs: explain that NoteStorage::new only fails on > MAX_NOTE_STORAGE_ITEMS, we always pass exactly NUM_STORAGE_ITEMS = 7, and the const _: () = assert! above proves the inequality at compile time. - mod.rs: explain that NoteAssets::new only fails on > MAX_NUM_ASSETS or on a duplicate, we always pass exactly one asset (covered by the const _: () = assert!), and the duplicate-detection loop skips index 0 so it never runs for a single-element vec. The old comments said 'unreachable per the const assert above' without spelling out WHY both failure conditions are ruled out. Future readers shouldn't have to dig into NoteStorage::new / NoteAssets::new to convince themselves.
…able!()) The SAFETY comment above each call already proves the path is unreachable; .unwrap() carries the same semantics with less ceremony and gives a more informative panic message (the actual NoteError) if the impossible ever happens.
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.
Follow-ups to the PSWAP note refactor, addressing the four review groups from upstream issue 0xMiden/protocol#3000, plus one fix surfaced by the review pass on this branch.
Commits
b8d78bf6refactor(pswap): switch public APIs to AssetAmount and NonZeroU32 (Group A)PswapNoteStorage::requested_asset_amount()returnsAssetAmount.PswapNote::create_argstakesAssetAmounton both args and is infallible.PswapNoteAttachment::newtakesdepth: NonZeroU32.parent_depthreturnsu32,next_depth()yieldsNonZeroU32with overflow check.depth == 0checks frompayback_note/remainder_note(now type-enforced).cf8fee4erefactor(pswap): hardenPswapNoteAttachmentparsing (Group B)TryFrom<&NoteAttachment>validates scheme,num_words == 1,AssetAmountrange, and non-zerou32depth.parent_depththrough the typed parse so malformed attachments degrade to0in one place.TryFromon every output (9 sites).a29e336bfeat(pswap.masm): assertnum_words == 1and depth fits inu32inget_current_depth(Group C)write_attachment_to_memory'snum_wordsagainstPSWAP_ATTACHMENT_NUM_WORDS = 1.parent_depthinto(lo, hi)andassertzthe highu32 == 0. Uses manual hi-check overu32assert.err=...so the named error surfaces verbatim at the test boundary (the VM wrapsu32assertmessages at runtime).PSWAP_ATTACHMENT_SCHEMEattachment.NoteAttachment::{with_words,with_word}and exercise both asserts.54d07fa4refactor(pswap): split into submodule, introduceOrderId, exposeAssetIdview (Group D)crates/miden-standards/src/note/pswap.rs(1430 lines) intopswap/{mod,storage,attachment,tests}.rs.git mvpreserves history on thepswap.rs → pswap/mod.rspath.OrderId(Felt)newtype used byPswapNote::order_idandPswapNoteAttachment{::new,::order_id}.PswapNoteStorage::requested_asset_id()returningAssetId(on-chain layout unchanged).PARENT_ATTACHMENT_DEPTH_OFFSETtopub(super)soattachment.rs'sTryFromcan consume it.OrderIdfromcrates/miden-standards/src/note/mod.rs.2b17b5fdfix(pswap): reject zero offered/requested amounts inPswapNote::buildcalculate_offered_for_requestedwould panic with a u128 div-by-zero on aPswapNotewhoserequested_amount == 0.PswapNote::build. Covers every constructor includingTryFrom<&Note>, which funnels through the builder.u128::div; this just moves the rejection to the Rust layer so callers get a typedNoteErrorinstead of a panic.Verification
cargo check --workspace --testsclean.cargo test -p miden-standards --lib pswap— 27 unit tests pass (21 baseline + 6 new forOrderIdround-trip,order_id == serial[1],requested_asset_idpacking, wrong-num-wordsTryFromrejection, and the two zero-amount builder guards).cargo test -p miden-testing --test lib -- pswap— 72 integration tests pass (70 baseline + 2 new for the MASMnum_wordsand depth asserts).pswap/attachment.rs,OrderId,requested_asset_id, hardenedTryFrom, zero-amount guards) is at or above 95%.Review
parent_depth == u32::MAXproducing unspendable outputs,depth == 0MASM-vs-Rust leniency gap) were classified as DoS / economic edge cases that are atomically reverted at the VM level and tracked separately, not blockers for this PR.Notes
nexton inicio's fork.nextata1be171b, which currently matches0xMiden/protocol next.Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests