Skip to content

PSwap Contract#2636

Open
VAIBHAVJINDAL3012 wants to merge 71 commits into0xMiden:nextfrom
inicio-labs:pswap-note
Open

PSwap Contract#2636
VAIBHAVJINDAL3012 wants to merge 71 commits into0xMiden:nextfrom
inicio-labs:pswap-note

Conversation

@VAIBHAVJINDAL3012
Copy link
Copy Markdown
Contributor

  • Add PSWAP (partially-fillable swap) note to miden-standards — a new note script that enables partial
    fills, creator reclaim, and inflight cross-swaps for decentralized asset exchange

    • MASM script (pswap.masm) uses 18-item KEY+VALUE storage layout, proportional price calculation with
      1e5 precision factor, and an early-return optimization for full fills to avoid integer truncation
    • Rust library (PswapNote) provides note creation, output note construction (P2ID payback + optional
      remainder), storage parsing, and calculate_offered_for_requested convenience method
    • 12 integration tests covering full fill, private fill, partial fill, inflight cross-swap, creator
      reclaim, invalid input rejection, multiple fill amounts, non-exact ratios, fuzz cases, and chained
      partial fills

    Test plan

    • cargo build -p miden-standards — MASM compiles into standards library
    • cargo test -p miden-standards pswap — 4 unit tests (script loading, tag construction, output
      calculation, storage parsing)
    • cargo test -p miden-testing pswap — 12 integration tests (full/partial/private fills, cross-swaps,
      reclaim, fuzz, chained fills)

Comment thread crates/miden-standards/src/note/pswap.rs Outdated
Copy link
Copy Markdown
Contributor

@PhilippGackstatter PhilippGackstatter left a comment

Choose a reason for hiding this comment

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

Looks good, thank you! I primarily looked at the Rust code for now and left a few comments, but on a high level:

  • I think we should use the builder-style way of creating PswapNote and PswapNoteStorage, since both have quite a few fields.
  • Some lints are failing (see CI or run make lint locally).
  • If this PR is ready for review, can we put it ouf of draft mode?
  • Nits: Would be nice to use attachment instead of aux, storage instead of inputs, sender instead of creator, etc.

Comment thread crates/miden-standards/src/note/pswap.rs Outdated
Comment thread crates/miden-standards/src/note/pswap.rs Outdated
Comment thread crates/miden-standards/src/note/pswap.rs Outdated
Comment thread crates/miden-standards/src/note/pswap.rs Outdated
Comment thread crates/miden-standards/src/note/pswap.rs Outdated
Comment thread crates/miden-standards/src/note/pswap.rs Outdated
Comment thread crates/miden-standards/src/note/pswap.rs Outdated
Comment thread crates/miden-standards/src/note/pswap.rs Outdated
Comment thread crates/miden-standards/src/note/pswap.rs Outdated
Comment thread crates/miden-standards/src/note/pswap.rs Outdated
Vaibhav Jindal and others added 14 commits March 25, 2026 09:14
…MS, remove create_ wrappers, add builder finish_fn

- Rename swapp_tag -> pswap_tag and SWAPp -> PSWAP throughout
- Rename NUM_ITEMS -> NUM_STORAGE_ITEMS for clarity
- Remove create_p2id_payback_note and create_remainder_note wrappers,
  make build_ functions public instead
- Compute p2id_tag inside build_p2id_payback_note from self.storage
- Add #[builder(finish_fn(vis = "", name = build_internal))] to PswapNote
…ctly

Replace all test helper wrappers with direct calls to library functions:
- create_pswap_note -> PswapNote::create()
- create_expected_pswap_p2id_note + create_expected_pswap_remainder_note -> pswap.execute()
- build_pswap_storage -> PswapNoteStorage::from_parts()
- Remove make_pswap_tag, make_note_assets, make_note_args, compute_p2id_tag_*
- Inline calculate_output_amount as PswapNote::calculate_output_amount()
- Replace storage layout list with markdown table
- Remove trivial "Returns the X" docs on simple getters
- Add # Errors sections where relevant
- Rewrite method docs to describe intent, not implementation
- Add one-line docs on From/TryFrom conversion impls
- Tighten PswapNote struct doc
- Rename RpoRandomCoin to RandomCoin (miden-crypto 0.23 rename)
- Store full ASSET_KEY instead of prefix/suffix to preserve callback
  metadata in faucet_id_suffix_and_metadata
- Replace create_fungible_asset calls with direct ASSET_KEY + manual
  ASSET_VALUE construction, avoiding the new enable_callbacks parameter
- Update hardcoded P2ID script root to match current P2idNote::script_root()
- Replace hardcoded P2ID script root with procref.p2id::main for
  compile-time resolution
- Default to full fill when both input and inflight amounts are zero
- Replace magic address 4000 with named P2ID_RECIPIENT_STORAGE constant
- Remove step numbering from comments, fix memory layout docs
- Rename P2ID_RECIPIENT_STORAGE to P2ID_RECIPIENT_SUFFIX/PREFIX
- Add METADATA_HEADER word layout docs with source reference
- Document attachment_scheme parameter in set_word_attachment call
- Add stack views after condition checks
…lper, simplify build_p2id_payback_note

Rename requested_key/requested_value/requested_amount to
requested_asset_key/requested_asset_value/requested_asset_amount for
clarity. Extract offered_asset_amount() helper on PswapNote to
deduplicate offered asset extraction. Simplify build_p2id_payback_note
to take fill_amount: u64 instead of aux_word: Word, constructing the
aux word internally.
- Use NoteTag::default() instead of NoteTag::new(0)
- Change swap_count from u64 to u16 with safe try_into conversion
- Rename p2id_tag to payback_note_tag, p2id_payback_note to payback_note
- Rename build_ prefix to create_ for consistency
- Rename aux_word to attachment_word
- Replace Felt::new with Felt::from where possible
- Rename inputs to note_storage in TryFrom impl
- Make create_payback_note and create_remainder_pswap_note private
- Add offered_faucet_id helper to replace unreachable!()
- Remove PswapNote::create, add public build() with validation on builder
- Add bon::Builder to PswapNoteStorage, remove new() and from_parts()
- Remove payback_note_tag field, compute from creator_account_id on the fly
- Fix clippy warnings (useless conversions, needless borrows)
- Update all unit and integration tests to use builder pattern
- Compare full faucet_id instead of just prefix in build validation
- Rename swap_note to pswap_note in integration tests for consistency
- Extract PswapNoteStorage builder into separate let bindings
- Replace manual current_swap_count counter with enumerate index
- Fix clippy useless_conversion and needless_borrow warnings
Copy link
Copy Markdown
Contributor

@PhilippGackstatter PhilippGackstatter left a comment

Choose a reason for hiding this comment

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

Thanks for the updates! Looks good overall.

I left a few comments regarding the core logic in Rust. I will take a look at the MASM implementation next.

Comment thread crates/miden-standards/asm/standards/notes/pswap.masm Outdated
Comment thread crates/miden-standards/asm/standards/notes/pswap.masm Outdated
Comment thread crates/miden-standards/src/note/pswap.rs Outdated
Comment thread crates/miden-standards/src/note/pswap.rs Outdated
Comment thread crates/miden-standards/src/note/pswap.rs
Comment thread crates/miden-standards/src/note/pswap.rs Outdated
Comment thread crates/miden-standards/src/note/pswap.rs Outdated
Comment thread crates/miden-standards/src/note/pswap.rs
Comment thread crates/miden-standards/src/note/pswap.rs Outdated
Comment on lines +561 to +580
# Calculate offered_out for input_amount
mem_load.AMT_REQUESTED_IN
mem_load.AMT_REQUESTED
mem_load.AMT_OFFERED
# => [offered, requested, input_amount]
exec.calculate_tokens_offered_for_requested
# => [input_offered_out]

mem_store.AMT_OFFERED_OUT_INPUT
# => []

# Calculate offered_out for inflight_amount
mem_load.AMT_REQUESTED_INFLIGHT
mem_load.AMT_REQUESTED
mem_load.AMT_OFFERED
# => [offered, requested, inflight_amount]

exec.calculate_tokens_offered_for_requested
# => [inflight_offered_out]
mem_store.AMT_OFFERED_OUT_INFLIGHT
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What's the advantage of calculating the input and inflight output amounts separately? We could save a few cycles if we added them together and called calculate_tokens_offered_for_requested only once? If there's a reason, it'd be great to document it in a comment here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We need to calculate the offered amount corresponding to the input_amount seperately, so that we can send that value to the consumer's vault.
We need to calculate the total offered out to create the Pswap note for the remaining asset.

…ve safety and docs

- PswapNoteStorage: store FungibleAsset instead of raw key/value Words
- PswapNote: store offered_asset as FungibleAsset instead of NoteAssets
- execute(): take Option<FungibleAsset> for input/inflight assets
- Split execute into execute() and execute_full_fill_network() for clarity
- create_tag(): accept &FungibleAsset instead of &Asset
- Eliminate all Felt::new usage (use Felt::from, Felt::try_from, + ONE)
- Add swap_count overflow protection via checked_add
- Add script root verification in TryFrom<&Note>
- Add MASM fill validation (fill_amount <= requested)
- Rename MASM constants: _INPUT -> _ITEM, SWAPP_ -> PSWAP_
- Make calculate_output_amount private, add precision doc comment
- Add doc comments on all public accessors and methods
- Add network account full fill test
- Document network transaction compatibility throughout
Copy link
Copy Markdown
Contributor

@PhilippGackstatter PhilippGackstatter left a comment

Choose a reason for hiding this comment

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

Looks good! I have gone through most of the MASM code, but not all yet.

I left a few questions/comments, primarily:

  • It would be great if we could have explicit branches in the code for the "input" and "inflight" logic.
  • We can reuse a few existing procedures from the standards/protocol library, e.g. p2id::new, account_id::is_equal.
  • The PSWAP note relies a lot on global memory and this makes the individual procedures a bit harder to understand and less reusable (because they have implicit assumptions about what is already loaded in memory). Ideally, we can pass parameters via the stack for more explicitness and more reusability.
    • One thing we recently discussed internally was potentially making note just a note script public but also its individual procedures, so users can more easily build their own flavor of something (e.g. a "pswap extended" note).

Comment thread crates/miden-standards/asm/standards/notes/pswap.masm Outdated
Comment thread crates/miden-standards/asm/standards/notes/pswap.masm Outdated
Comment thread crates/miden-standards/asm/standards/notes/pswap.masm Outdated
Comment thread crates/miden-standards/asm/standards/notes/pswap.masm Outdated
Comment thread crates/miden-standards/asm/standards/notes/pswap.masm
Comment thread crates/miden-standards/asm/standards/notes/pswap.masm Outdated
Comment thread crates/miden-standards/asm/standards/notes/pswap.masm Outdated
Comment thread crates/miden-standards/asm/standards/notes/pswap.masm Outdated
Comment thread crates/miden-standards/asm/standards/notes/pswap.masm Outdated
Comment thread crates/miden-standards/asm/standards/notes/pswap.masm
`calculate_tokens_offered_for_requested` previously used a fixed-point
1e5 precision factor with two separate branches (offered >= requested
vs requested > offered) plus a full-fill early return, to approximate
`offered * fill / requested` without overflowing u64 intermediates.

That design had three problems:

- Four `u64::wrapping_mul` calls silently produced wrong results for
  inputs where `x * FACTOR >= 2^64`, capping each operand at about
  1.84e14 — roughly 0.000184 whole tokens for any 18-decimal asset.
- The two-branch structure was fragile and harder to reason about than
  a single linear formula.
- The FACTOR-scaled intermediate introduced rounding error in `ratio`
  that then got amplified by `fill`.

Replace the whole proc with one linear path:

  product = u64::widening_mul(offered, fill_amount)   # -> u128
  quot    = u128::div(product, requested_u128)        # -> u128
  assert  q.hi_limbs == 0                              # payout fits in u64
  return  q.lo_limbs combined back into a felt

miden-core-lib 0.22 already exposes `u128::div` (advice-provider
assisted, same host-compute-and-verify pattern as `u64::div`), so no
custom division implementation is needed.

Properties of the new version:
- Exact integer precision (one floor division at the end, no FACTOR
  rounding).
- Each operand can go up to `FungibleAsset::MAX ≈ 2^63`, so 18-decimal
  tokens now work at realistic volumes (~9.2 billion whole tokens
  per swap) — a ~50,000x improvement in dynamic range.
- No branching on the offered/requested relationship. No full-fill
  early return. One generic formula that also handles the
  `fill_amount == requested` case trivially.
- All wrapping_mul references gone, closing the last deferred theme-1
  item from the PR review.

Also mirror the same change on the Rust side: `calculate_output_amount`
now just does `(offered as u128) * (fill as u128) / (requested as u128)`
and `try_from`s back to u64, matching MASM exactly. Scrub the old
PRECISION_FACTOR constant, the two-branch Rust logic, and the FACTOR
doc block.

The 8 in-crate unit tests and all 60 pswap script tests (including
the 27-case hand-picked ratio regression suite and two seeded
30-iteration fuzzers) still pass.
Copy link
Copy Markdown
Contributor

@partylikeits1983 partylikeits1983 left a comment

Choose a reason for hiding this comment

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

Thank you for addressing many of the comments I had yesterday! Still a few more formatting issues & also we still need to remove the call to truncate_stack.

For testing purposes you can use sdepth debug.stack drop throughout the execution of the note to see where the stack depth does not line up with the stack comments.

Comment thread crates/miden-standards/asm/standards/notes/pswap.masm Outdated
Comment thread crates/miden-standards/asm/standards/notes/pswap.masm Outdated
Comment thread crates/miden-standards/asm/standards/notes/pswap.masm Outdated
Comment thread crates/miden-standards/asm/standards/notes/pswap.masm Outdated
Comment thread crates/miden-standards/asm/standards/notes/pswap.masm
Comment thread crates/miden-standards/asm/standards/notes/pswap.masm
Comment thread crates/miden-standards/asm/standards/notes/pswap.masm
Comment thread crates/miden-standards/asm/standards/notes/pswap.masm Outdated
Comment thread crates/miden-standards/asm/standards/notes/pswap.masm Outdated
Comment thread crates/miden-standards/asm/standards/notes/pswap.masm Outdated
VAIBHAVJINDAL3012 and others added 10 commits April 16, 2026 00:20
- `calculate_output_amount` doc: fix a mismatched paren and the empty
  blank line between doc and fn signature that triggered clippy's
  `empty_line_after_doc_comments` (caught by CI after the last push).
- `pswap_note_alice_reconstructs_and_consumes_p2id`: build the
  `PswapNote` via the builder, call `pswap.clone().into()` to get the
  protocol `Note`, and keep `pswap` in scope for `.execute(...)`. Drops
  the `PswapNote::try_from(&pswap_note)?` roundtrip the reviewer flagged
  on this exact test as a nit.
- `pswap_chained_partial_fills_test`: same pattern — replace the manual
  `Note::new(NoteAssets / NoteMetadata / NoteRecipient)` construction
  with `PswapNote::builder().serial_number(current_serial)...build()?`
  + `pswap.clone().into()`. The raw-Note construction was only there to
  inject a specific serial per chain position, which the builder
  already supports. Also drops the dependent `PswapNote::try_from`
  roundtrip.
- `pswap_fill_test`: bump `AMOUNT_SCALE` from 10^12 to 10^18 so the
  happy-path test exercises the MASM u128 calculation at realistic
  18-decimal token magnitudes. Base values adjusted (offered=8,
  requested=4, fills=3/4) so every amount stays under AssetAmount::MAX
  ≈ 9.22 × 10^18. Previously the 10^12 scale was chosen to stay under
  the old FACTOR=1e5 cap; now that the u128 rewrite lifted that cap,
  the test can stress the calculation at the actual wei-equivalent
  scale the reviewer asked for.
- New `pswap_attachment_layout_matches_masm_test`: dedicated
  regression test for the shared P2ID and remainder attachment-word
  layout. Does a partial fill, then explicitly asserts the executed
  transaction's P2ID attachment equals `[fill_amount, 0, 0, 0]` and
  the remainder attachment equals `[amt_payout, 0, 0, 0]`, and
  cross-checks both against the Rust-predicted attachments. Fires if
  either MASM or Rust drifts the load-bearing felt off `Word[0]`.
- Apply the "blank line after every `# => [...]` stack comment"
  convention across pswap.masm where it was missing — one reviewer
  tends to leave this as a nit so sweep the whole file for
  consistency rather than wait for the comments.
Add `PswapNote::create_args(account_fill: u64, note_fill: u64) ->
Result<Word, NoteError>` so downstream consumers building PSWAP
`NOTE_ARGS` words don't need to know the `[account_fill, note_fill,
0, 0]` layout by hand. Returns a `Result` instead of panicking so the
underlying `Felt::try_from` conversion errors propagate cleanly
through the standard `NoteError` path — although for any amount that
fits in `FungibleAsset::MAX_AMOUNT` this cannot actually fail, the
conversion is surfaced explicitly rather than hidden behind an
`.expect(...)` in a public API.

Drop the local `pswap_args` test helper — every call site now uses
`PswapNote::create_args(...)?` directly, matching the reviewer's
suggestion that the layout helper live on `PswapNote` rather than
being reimplemented in each consumer.

All 61 pswap script tests pass with the new signature (twelve call
sites updated to propagate the Result via `?`).
Co-authored-by: Alexander John Lee <77119221+partylikeits1983@users.noreply.github.com>
Co-authored-by: Alexander John Lee <77119221+partylikeits1983@users.noreply.github.com>
Co-authored-by: Alexander John Lee <77119221+partylikeits1983@users.noreply.github.com>
Co-authored-by: Alexander John Lee <77119221+partylikeits1983@users.noreply.github.com>
Co-authored-by: Alexander John Lee <77119221+partylikeits1983@users.noreply.github.com>
Co-authored-by: Alexander John Lee <77119221+partylikeits1983@users.noreply.github.com>
Co-authored-by: Alexander John Lee <77119221+partylikeits1983@users.noreply.github.com>
@VAIBHAVJINDAL3012
Copy link
Copy Markdown
Contributor Author

Big change in calculations
Replaced the FACTOR = 1e5 fixed-point workaround with native u64::widening_mul + u128::div (now available in miden-core-lib 0.22). Both branches and the full-fill shortcut collapse into a single linear formula on both sides:

MASM (calculate_tokens_offered_for_requested):
widening_mul(offered, fill) → u128 product
u128::div / requested (zero-extended) → u128 quotient
assert upper 64 bits == 0 → ERR_PSWAP_PAYOUT_OVERFLOW
reconstruct low 64 bits as felt

Two fixes triggered by `make check-features` / `clippy -D warnings`
and the full pswap test suite:

- `tests/scripts/pswap.rs`: drop `NoteAssets`, `NoteMetadata`,
  `NoteRecipient`, `NoteStorage` from the `miden_protocol::note`
  import list. Leftovers from the theme-5 chained-fills refactor that
  switched from low-level `Note::new(NoteAssets, NoteMetadata,
  NoteRecipient)` construction to `PswapNote::builder()`. CI compiles
  with `-D warnings` so the unused-imports warning was fatal.

- `asm/standards/notes/pswap.masm`: remove a duplicate
  `exec.calculate_tokens_offered_for_requested` line that had crept
  into the note-fill payout path in one of the upstream edits, plus a
  duplicate stack comment above the account-fill payout block. With
  the duplicate `exec`, the second call read garbage off a stack that
  only held the first call's payout — specifically a zero divisor,
  which surfaced as `u128::div` "division by zero" and failed 57/61
  pswap tests.

All 61 pswap script tests pass after the fix, and
`cargo check --all-targets --all-features` on miden-testing is clean.
Copy link
Copy Markdown
Contributor

@PhilippGackstatter PhilippGackstatter left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thank you for addressing all the comments and adding the account + note fill test, it looks great!

I left a few more comments regarding:

  • checking calculated asset amounts fit into fungible asset amounts
  • Derive the payback note tag from the creator ID instead of passing it via storage.
  • Introduce dedicated attachment constructors on PswapNote.

After these I think everything looks good.

In a subsequent PR or issue, we may want to discuss how clients are expected to discover payback and remainder notes exactly, because I'm not sure yet if we need to standardize an attachment scheme for the payback and remainder attachments, or not. This may need a bit of discussion, so for now we can leave these as NoteAttachmentScheme::none.

Comment thread crates/miden-standards/asm/standards/notes/pswap.masm
Comment thread crates/miden-standards/asm/standards/notes/pswap.masm
Comment thread crates/miden-standards/asm/standards/notes/pswap.masm Outdated
Comment on lines 130 to 131
// Payback tag [4]
Felt::from(storage.payback_note_tag()),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we should either:

  • Make the payback note tag length configurable on the Rust side, so creators can configure what they need,
  • Or derive the tag directly in MASM from the creator account ID
    mem_load.PSWAP_CREATOR_PREFIX_ITEM
    exec.::miden::standards::note_tag::create_account_target
    # => [creator_note_tag]
    

As it is now, we derive a note tag in Rust that could also be derived in MASM and it requires an otherwise unnecessary storage item.

I would go with the second option, since the configuration is probably not necessary. The creator can derive the exact expected payback note ID in any case, so it shouldn't even need to rely on the tag. So, we could even remove the tag entirely, but for uniform handling in the client, it may be nice to have it regardless.

If we do this, we should make sure that the payback note tag derived in Rust matches the tag derived in Rust by adding an assertion to an existing test.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, we can go with option 2 as tag is not configurable in our case.
It just came to my mind that instead of doing it in Pswap note, we can add this in p2id:new where we can directly calculate the tag

@Locals(2)
pub proc new
# => [target_id_suffix, target_id_prefix, note_type, SERIAL_NUM]

loc_store.TARGET_ACCOUNT_ID_SUFFIX_PTR
# => [target_id_prefix, note_type, SERIAL_NUM]

dup loc_store.TARGET_ACCOUNT_ID_PREFIX_PTR
# => [target_id_prefix, note_type, SERIAL_NUM]

exec.note_tag::create_account_target
# => [tag, note_type, SERIAL_NUM]

movdn.5 movdn.5
# => [SERIAL_NUM, tag, note_type]

procref.main
# => [SCRIPT_ROOT, SERIAL_NUM, tag, note_type]

swapw
# => [SERIAL_NUM, SCRIPT_ROOT, tag, note_type]

push.2 locaddr.STORAGE_PTR
# => [storage_ptr, num_storage_items=2, SERIAL_NUM, SCRIPT_ROOT, tag, note_type]

exec.note::build_recipient
# => [RECIPIENT, tag, note_type]

movup.5 movup.5
# => [tag, note_type, RECIPIENT]

exec.output_note::create
# => [note_idx]

end

Comment thread crates/miden-standards/src/note/pswap.rs
Comment thread crates/miden-standards/src/note/pswap.rs Outdated
Comment thread crates/miden-testing/tests/scripts/pswap.rs
Comment thread crates/miden-testing/tests/scripts/pswap.rs
- Derive payback note tag from creator ID in MASM via
  note_tag::create_account_target instead of storing in note storage,
  reducing storage from 8 to 7 items.
- Add dedicated attachment constructors (payback_attachment,
  remainder_attachment) on PswapNote for clarity and reuse.
- Validate computed amounts as valid fungible asset amounts:
  - calculate_output_amount now returns Result and checks via AssetAmount.
  - MASM assert_valid_asset_amount proc checks u64 values against
    FUNGIBLE_ASSET_MAX_AMOUNT before felt conversion to prevent silent
    wrapping, applied to both fill sum and payout quotient.
- Fix P2ID comment nit per reviewer suggestion.
- Remove unused NoteTag import from p2id test.
Copy link
Copy Markdown
Contributor

@PhilippGackstatter PhilippGackstatter left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Left a comment to simplify assert_valid_asset_amount. Re adding it to the protocol library: I think this needs some consideration because in the protocol asset amounts are felts, but we also encourage users to use u64 arithmetic on asset amounts, so it's not clear what the way forward is exactly.

I would wait for @partylikeits1983's approval before merging.

Comment thread crates/miden-standards/asm/standards/notes/pswap.masm
Comment thread crates/miden-standards/asm/standards/notes/pswap.masm
Comment thread crates/miden-standards/src/note/pswap.rs
- Replace manual hi/lo branch logic with exec.u64::lte
- Rename calculate_tokens_offered_for_requested to calculate_output_amount (matches Rust)
- Simplify tag derivation in create_p2id_note: swap+dup+swap+movup.2 → dup.1+movdn.2
@VAIBHAVJINDAL3012
Copy link
Copy Markdown
Contributor Author

Should I add the orderID in the attachment as suggested by @bobbinth #2782 in this same PR or should we merge this PR first and then raise the small PR over it for the orderID?

Copy link
Copy Markdown
Contributor

@partylikeits1983 partylikeits1983 left a comment

Choose a reason for hiding this comment

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

I think this is ready to merge once the comments below are resolved. I went through the tests again. I don't think there are tests which test that the PSWAP note fails as expected (comments below).

Once these items are addressed, I think this is ready to merge.

Comment thread crates/miden-standards/asm/standards/notes/pswap.masm Outdated
Comment thread crates/miden-standards/asm/standards/notes/pswap.masm Outdated
Comment thread crates/miden-standards/asm/standards/notes/pswap.masm Outdated
Comment thread crates/miden-standards/asm/standards/notes/pswap.masm
Comment on lines +171 to +174
movup.3 eq.0 assert.err=ERR_PSWAP_PAYOUT_OVERFLOW
# => [q0, q1, q2]

movup.2 eq.0 assert.err=ERR_PSWAP_PAYOUT_OVERFLOW
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is ERR_PSWAP_PAYOUT_OVERFLOW ever tested?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This case is not possible, but put the check mainly for the security purpose as suggested by Phillip.

@PhilippGackstatter
Copy link
Copy Markdown
Contributor

Should I add the orderID in the attachment as suggested by @bobbinth #2782 in this same PR or should we merge this PR first and then raise the small PR over it for the orderID?

I think it'd be great to put this into a subsequent PR.

VAIBHAVJINDAL3012 and others added 5 commits April 18, 2026 01:56
Co-authored-by: Alexander John Lee <77119221+partylikeits1983@users.noreply.github.com>
Co-authored-by: Alexander John Lee <77119221+partylikeits1983@users.noreply.github.com>
Co-authored-by: Alexander John Lee <77119221+partylikeits1983@users.noreply.github.com>
…unt validation

Converts pswap_note_invalid_input_test to rstest with three named cases:
- fill_exceeds_requested (existing)
- fill_sum_u64_overflow: both fills at 2^63, sum overflows u64 → ERR_PSWAP_FILL_SUM_OVERFLOW
- fill_sum_exceeds_max_asset_amount: both fills at MAX_AMOUNT, sum > MAX_AMOUNT → ERR_PSWAP_NOT_VALID_ASSET_AMOUNT
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.

4 participants