feat(standards): treat PSWAP requested amount as a minimum#9
Closed
VAIBHAVJINDAL3012 wants to merge 3 commits into
Closed
feat(standards): treat PSWAP requested amount as a minimum#9VAIBHAVJINDAL3012 wants to merge 3 commits into
VAIBHAVJINDAL3012 wants to merge 3 commits into
Conversation
Previously the PSWAP note reverted when the total fill (account_fill + note_fill) exceeded the requested amount. This makes `requested` a minimum rather than an exact cap: an over-fill is accepted and treated as a full take. - Payout divisor is `denom = max(total_fill, requested)`, so an over-fill divides by the fill itself and pays out the whole offered side. - No remainder note is created on a full or over-fill: the partial-fill check is `total_in < total_requested` (was `!=`). - The creator banks the surplus requested tokens via the P2ID payback, which carries the full fill amount. - Removes the `ERR_PSWAP_FILL_EXCEEDS_REQUESTED` cap. MASM and Rust mirror each other (denom selection, no-remainder-on-overfill, payout read from on-chain attachments). Removing the cap also removes accidental-overpay protection: a filler who over-pays gets no refund and no extra offered tokens, so callers must compute the intended fill amount. Tests: single-sided over-fill payback round-trip, mixed account+note over-fill with rounding-dust attribution, a calculate_offered_for_requested over-fill unit case; the fill-exceeds-requested error case is removed.
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughPSWAP now treats requested amounts as minimums, computes payouts with ChangesPSWAP over-fill support
Sequence Diagram(s)sequenceDiagram
participant Consumer
participant PswapNoteExecute as PswapNote::execute
participant ExecutePswap as execute_pswap
participant CalculateOutputAmount as calculate_output_amount
participant CreateP2idNote as create_p2id_note
Consumer->>PswapNoteExecute: fill_amount, requested_amount
PswapNoteExecute->>ExecutePswap: execute PSWAP
ExecutePswap->>CalculateOutputAmount: account_fill payout with max(fill, requested)
ExecutePswap->>CalculateOutputAmount: note_fill payout with max(fill, requested)
ExecutePswap->>CreateP2idNote: creator payback when fill < requested
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/miden-testing/tests/scripts/pswap.rs (1)
327-347: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert Bob receives the full offered side in this over-fill test.
This test documents “Bob takes the whole 50 USDC offered side”, but currently only verifies the payback/no-remainder side. Add a vault-patch assertion after the first execution so the single-sided full-take behavior is covered directly.
Suggested assertion
let executed_transaction = tx_context.execute().await?; + let vault_patch = executed_transaction.account_patch().vault(); + assert_vault_patch( + vault_patch, + [ + FungibleAsset::new(usdc_faucet.id(), 50)?, + FungibleAsset::new(eth_faucet.id(), 0)?, + ], + ); mock_chain.add_pending_executed_transaction(&executed_transaction)?;🤖 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 327 - 347, The over-fill test currently only checks the payback and remainder behavior, so it does not directly assert that Bob actually receives the full offered side. Update the pswap.rs test around the tx_context.execute() flow to add a vault-patch assertion immediately after the first execution, using the existing executed_transaction/mocked chain setup and the same full-take scenario, so the test explicitly verifies Bob gets the entire 50 USDC offered side.
🤖 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.
Nitpick comments:
In `@crates/miden-testing/tests/scripts/pswap.rs`:
- Around line 327-347: The over-fill test currently only checks the payback and
remainder behavior, so it does not directly assert that Bob actually receives
the full offered side. Update the pswap.rs test around the tx_context.execute()
flow to add a vault-patch assertion immediately after the first execution, using
the existing executed_transaction/mocked chain setup and the same full-take
scenario, so the test explicitly verifies Bob gets the entire 50 USDC offered
side.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d097110f-84b8-4792-b3f2-3eefdd51c5a1
📒 Files selected for processing (3)
crates/miden-standards/asm/standards/notes/pswap.masmcrates/miden-standards/src/note/pswap.rscrates/miden-testing/tests/scripts/pswap.rs
VAIBHAVJINDAL3012
left a comment
VAIBHAVJINDAL3012
left a comment
Author
There was a problem hiding this comment.
Isn't the case the we might have 1 surpplus asset remaining....
in (note_fill/total*offered...in this case???
| # calculate_output_amount locals | ||
| const CALC_FILL_AMOUNT = 0 | ||
| const CALC_REQUESTED = 1 | ||
| const CALC_DENOM = 1 |
Author
There was a problem hiding this comment.
Can we have better name?
Comment on lines
+537
to
+539
| #! `requested_amount` is a minimum: the total fill (account_fill + note_fill) may exceed it | ||
| #! from any source, paying out the whole offered side (no remainder) and banking the surplus | ||
| #! to the creator. Fills at or below it are proportional and leave a remainder note. |
Author
There was a problem hiding this comment.
Write it in clear way p;lease
Comment on lines
+614
to
+624
| dup.1 dup.1 lt | ||
| # => [fill_lt_requested, requested, fill_amount] | ||
|
|
||
| if.true | ||
| swap drop | ||
| # => [requested] | ||
| else | ||
| drop | ||
| # => [fill_amount] | ||
| end | ||
| # => [denom] |
Author
There was a problem hiding this comment.
write max function, that is more clear.....or check do we have it masm lib.
| dup.1 dup.1 neq | ||
| # => [is_partial, total_requested, total_in] | ||
| dup.1 dup.1 lt | ||
| # => [is_partial, total_requested, total_in] (is_partial = total_in < total_requested) |
Author
There was a problem hiding this comment.
remove this is_partial comment please
Comment on lines
+530
to
+532
| /// `requested_amount` is a minimum: an over-fill (`fill_amount` greater than the requested | ||
| /// amount) takes the whole offered side, so the divisor is `max(fill_amount, requested)` | ||
| /// (see [`Self::execute`]). |
| /// MASM side. | ||
| /// Computes a fill's proportional share of the offered tokens: | ||
| /// `floor((offered_total * fill_amount) / denominator)`, computed via a u128 intermediate, | ||
| /// mirroring `u64::widening_mul` + `u128::div` on the MASM side. |
Author
There was a problem hiding this comment.
remove this mirrioring comment pleaseee
| /// | ||
| /// Net effect: Charlie -20 ETH / +28 USDC; two P2ID notes, no remainder. | ||
| #[tokio::test] | ||
| async fn pswap_note_combined_over_fill_test() -> anyhow::Result<()> { |
Author
There was a problem hiding this comment.
only this test wou;d be enough, previous test is waste..........for this feature this test wouljd fine.......i wouljd say add rtest over it which cover all the cases, i guess we have exixting test too, there we can use the rtest right??.......even remove both og these testsss???
Comment on lines
+1041
to
+1054
| fn calculate_offered_for_requested_over_fill() { | ||
| // Offered 50 for a minimum of 25 requested. `requested` is a floor, so the divisor is | ||
| // `max(fill, requested)`: any fill at or above the minimum takes the whole offered side. | ||
| let offered = FungibleAsset::new(dummy_faucet_id(0xaa), 50).unwrap(); | ||
| let requested = FungibleAsset::new(dummy_faucet_id(0xbb), 25).unwrap(); | ||
| let (pswap, _note) = build_pswap_note(offered, requested, dummy_creator_id()); | ||
|
|
||
| // Below the minimum stays proportional: floor(50 * 5 / 25) = 10. | ||
| assert_eq!(pswap.calculate_offered_for_requested(5).unwrap(), 10); | ||
| // Exact fill and any over-fill both yield the full offered amount. | ||
| assert_eq!(pswap.calculate_offered_for_requested(25).unwrap(), 50); | ||
| assert_eq!(pswap.calculate_offered_for_requested(30).unwrap(), 50); | ||
| assert_eq!(pswap.calculate_offered_for_requested(50).unwrap(), 50); | ||
| } |
Author
There was a problem hiding this comment.
again rtest or added case on earlier such function would be enough
we can remove it thennn
- Extract a `max` helper proc; `execute_pswap` calls `exec.max` instead of the inline lt/if-else to pick `denominator = max(fill, requested)`. - Rename `CALC_DENOM`/`EXEC_AMT_DENOM` to `*_DENOMINATOR`, matching the Rust mirror and spelling out the abbreviation. - Rewrite the `execute_pswap` doc with the two fill cases stated explicitly, and note that two-sided over-fill rounding dust (<=1 token) is absorbed by the cross-swap counterparty, never minted or stranded. - Drop the redundant inline `is_partial` annotation. - Test: assert the consumer takes the whole offered side on a single-sided over-fill (per CodeRabbit). No behaviour change: the max extraction is behaviour-equivalent. 10 standards unit + 69 pswap integration tests pass; fmt clean.
Comment on lines
+561
to
+563
| #! remainder is created. A single-sided fill sends it all to the consumer; a two-sided fill | ||
| #! credits the consumer their `account_fill` share and the cross-swap counterparty absorbs | ||
| #! the rest. Either way the creator banks the surplus requested tokens. |
Author
There was a problem hiding this comment.
Don't use single sided or double sided......
Account fill, note fill or if note or accouniut fill together.
Comment on lines
+634
to
+635
| # full_fill_amount = max(fill, requested). For an over-fill (fill > requested) this is the | ||
| # fill itself, so the whole offered side is paid out and no remainder note is created. |
Author
There was a problem hiding this comment.
what is the use fo this sentrnece here....
I think covered earlier in description in my opnion.
Comment on lines
631
to
652
| /// Integration test for a PSWAP fill that uses **both** `account_fill` and | ||
| /// `note_fill` on the same note in the same transaction. | ||
| /// | ||
| /// Setup: | ||
| /// - Alice's pswap: 100 USDC offered for 50 ETH requested (ratio 2:1). | ||
| /// - Bob's pswap: 30 ETH offered for 60 USDC requested (ratio 1:2). | ||
| /// - Charlie has 20 ETH in vault. | ||
| /// | ||
| /// Charlie consumes both notes in one tx: | ||
| /// - Alice's: `account_fill = 20 ETH` (debited from his vault) | ||
| /// + `note_fill = 30 ETH` (sourced from inflight, produced by Bob's pswap) | ||
| /// → 50 ETH total (full fill). Payout split: | ||
| /// - 40 USDC → Charlie's vault (account_fill path) | ||
| /// - 60 USDC → inflight (note_fill path, consumed by Bob's pswap) | ||
| /// - Bob's: `note_fill = 60 USDC` (sourced from inflight, produced by Alice's pswap) → 60 USDC | ||
| /// total (full fill). Payout: 30 ETH → inflight (matches Alice's note_fill consumption above). | ||
| /// Cross-swap that fills Alice's PSWAP from both `account_fill` (Charlie's vault) and `note_fill` | ||
| /// (Bob's offered leg) in one transaction. `requested_amount` is a minimum, so this covers an exact | ||
| /// full fill and an over-fill: | ||
| /// - `full_fill`: account_fill 20 + note_fill 30 = 50 = Alice's 50 ETH minimum. The 100 USDC | ||
| /// offered splits 40 (Charlie) / 60 (Bob); two P2IDs, no remainder. | ||
| /// - `over_fill`: account_fill 20 + note_fill 50 = 70 > 50. The 100 USDC splits against denom=70: | ||
| /// floor(100*20/70)=28 to Charlie, residual 72 to Bob (proportional note share 71 + 1 rounding | ||
| /// unit). Alice's P2ID carries the full 70 ETH (she banks the surplus); no remainder. | ||
| /// |
Author
There was a problem hiding this comment.
no need to make that big change change in commet, small change over the existing. comment should work fine.
| // Bob's pswap: 30 ETH offered for 60 USDC requested. | ||
| // Charlie consumes both; his vault supplies 20 ETH (account_fill) and | ||
| // the other 30 ETH is sourced from Bob's offered leg via note_fill. | ||
| // Alice offers 100 USDC for a 50 ETH minimum. Bob offers exactly the note_fill leg's ETH and |
Author
There was a problem hiding this comment.
Does this hardcoded numner make sense? as we use rtest case now?
Address review feedback on PR #9: - Rename the payout divisor `full_fill_amount` -> `fill_reference` across the MASM (CALC_/EXEC_AMT_ constants) and the Rust mirror. - MASM: drop single/two-sided wording in the execute_pswap doc (use account_fill/ note_fill); trim the inline `fill_reference = max(...)` comment that duplicated the proc doc. - Tests: shrink the combined-fill rstest doc and de-hardcode the per-case numbers in its body comments.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Makes the PSWAP note's
requestedamount a minimum rather than an exact cap. Previously the note reverted when the total fill (account_fill + note_fill) exceededrequested; now an over-fill is accepted and treated as a full take.Behaviour
denom = max(total_fill, requested). For any fill at or belowrequestedthis isrequested(unchanged: proportional, leaves a remainder); for an over-fill it is the fill itself, so the consumer takes the whole offered side and no remainder note is created.is_partialcheck changed fromtotal_in != total_requestedtototal_in < total_requested. This is required, not cosmetic: with!=, an over-fill would computerequested - total_inas a field underflow and emit a bogus remainder.ERR_PSWAP_FILL_EXCEEDS_REQUESTEDerror.The MASM and the Rust mirror stay in lockstep: same
denomselection, same no-remainder-on-over-fill, and the off-chain reconstruction reads fill/payout from the on-chain attachments rather than recomputing them.Design note (please sign off)
Removing the cap also removes accidental-overpay protection. By design, a single-sided over-fill is strictly worse for the filler: they pay the surplus and receive no extra offered tokens and no refund. Wallets/solvers must compute the intended fill amount. Flagging for a conscious decision, not as a defect.
Tests
pswap_overfill_single_sided_reconstructs_payback— single-sided over-fill; payback round-trips (reconstruct + consume), no remainder. Asserts the Rustexecute()prediction matches the on-chain output note, and that the consumer takes the whole offered side.pswap_note_combined_over_fill_test— mixedaccount_fill+note_fillover-fill across two paired notes; rounding dust to the counterparty; no remainder.calculate_offered_for_requested_over_fillunit case, plus thecalculate_output_amount(100, 100, 100) == 100boundary.fill_exceeds_requestederror case.Verified locally: 10/10 standards unit tests, 69/69 + 2 fuzz pswap integration tests, nightly fmt clean. Two independent reviews (arithmetic audit + principal-engineer) found no correctness issues — conservation
payout_account + payout_note <= offeredholds by construction, and two-sided over-fill rounding dust is in {0, 1} and absorbed by the counterparty.Summary by CodeRabbit
New Features
Bug Fixes
Tests