feat: support atomic cross-asset swap deposits on payment streams - #573
Conversation
|
@collinsezedike Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
|
Note on local verification: getting `cargo test` to run at all required working around a pre-existing, unrelated `Cargo.lock` resolution issue: `ed25519-dalek 3.0.0` (pulled in transitively via `soroban-env-host`'s testutils feature) requires `rand_core 0.10`'s `CryptoRng` trait, but the resolved `rand_chacha 0.3.1` only implements the older `rand_core 0.6` traits, so `soroban-env-host` fails to compile out of the box on this toolchain. I fixed it locally with: |
📝 WalkthroughWalkthroughThe payment-stream contract adds configurable cross-asset swaps for deposits. It validates provider responses, stream state, slippage, and capacity before updating balances and metrics. Tests and snapshots cover successful and rejected swap deposits. ChangesCross-Asset Swap Deposits
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant PaymentStreamContract
participant SwapProvider
participant DestinationToken
Caller->>PaymentStreamContract: deposit_with_swap
PaymentStreamContract->>SwapProvider: Transfer source asset
PaymentStreamContract->>SwapProvider: Execute swap
SwapProvider->>DestinationToken: Transfer destination asset
DestinationToken-->>PaymentStreamContract: Report balance delta
PaymentStreamContract-->>Caller: Credit stream and return amount
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 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 (1)
contracts/payment-stream/src/test.rs (1)
1846-1865: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winContract's own slippage check (balance-delta based) is never exercised by tests.
MockSwapProvider::swappanics internally wheneveramount_out < min_amount_out, sotest_deposit_with_swap_slippage_exceededonly ever exercises the mock's own guard — control never returns todeposit_with_swap's independentif amount_out < min_amount_out { panic_with_error!(...) }check in lib.rs. If that check regressed, no current test would catch it.Consider adding a mock provider mode (e.g. a
set_underdeliverflag) that successfully transfers less thanmin_amount_outwithout panicking, so the contract's own defensive check gets direct coverage.Also applies to: 2036-2069
🤖 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 `@contracts/payment-stream/src/test.rs` around lines 1846 - 1865, The MockSwapProvider::swap test double currently panics on slippage, preventing deposit_with_swap from exercising its own balance-delta validation. Add a configurable underdelivery mode, such as a set_underdeliver flag, that transfers an amount below min_amount_out without panicking, then update test_deposit_with_swap_slippage_exceeded to enable it and assert the contract’s panic_with_error! path is triggered.
🤖 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 `@contracts/payment-stream/src/lib.rs`:
- Around line 349-406: Update deposit_with_swap to prevent stale stream state
from being persisted after external calls: before calculating and writing the
final balance, re-read the stream using stream_id, validate that its status and
relevant fields still match the initial snapshot, and reject if reentrancy
mutated it; apply the swap amount to the fresh state and persist that validated
record. Ensure reentrant updates from swap_client.swap or source_client.transfer
cannot be overwritten by the outer call.
---
Nitpick comments:
In `@contracts/payment-stream/src/test.rs`:
- Around line 1846-1865: The MockSwapProvider::swap test double currently panics
on slippage, preventing deposit_with_swap from exercising its own balance-delta
validation. Add a configurable underdelivery mode, such as a set_underdeliver
flag, that transfers an amount below min_amount_out without panicking, then
update test_deposit_with_swap_slippage_exceeded to enable it and assert the
contract’s panic_with_error! path is triggered.
🪄 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: 29eb5571-eb84-4050-be3a-02a5d79ee1f3
📒 Files selected for processing (11)
contracts/payment-stream/src/lib.rscontracts/payment-stream/src/test.rscontracts/payment-stream/test_snapshots/test/test/test_deposit_with_swap_exceeds_total.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_deposit_with_swap_inactive_stream_rejected.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_deposit_with_swap_provider_not_set.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_deposit_with_swap_respects_actual_amount_received.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_deposit_with_swap_same_asset_rejected.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_deposit_with_swap_slippage_exceeded.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_deposit_with_swap_success.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_get_swap_provider_roundtrip.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_set_swap_provider_unauthorized.1.json
…vel slippage check
|
Addressed both in the latest commit:
49/49 tests pass, wasm build still clean with zero warnings. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@contracts/payment-stream/src/lib.rs`:
- Around line 371-372: Update the stream swap flow around the pre-swap snapshot
and the destination balance-delta calculation: capture the complete
accounting-relevant stream state, including withdrawn_amount (or the full
stream/mutation nonce), immediately after external calls, then validate it
before deriving amount_out. Ensure any authorized reentrant withdrawal returns
ReentrantStateChange before checked_sub or output calculation, and add a
regression test covering that withdrawal path.
In `@contracts/payment-stream/src/test.rs`:
- Around line 2087-2127: Strengthen
test_deposit_with_swap_slippage_exceeded_by_contract_check by matching the
failed try_deposit_with_swap result to the expected Error::SlippageExceeded
contract error. Capture the sender, swap provider, and contract token balances
and the stream balance before the call, then assert all remain unchanged
afterward to verify rollback.
🪄 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: 47b9ba2a-1864-472f-842a-c03d5b400bcf
📒 Files selected for processing (3)
contracts/payment-stream/src/lib.rscontracts/payment-stream/src/test.rscontracts/payment-stream/test_snapshots/test/test/test_deposit_with_swap_slippage_exceeded_by_contract_check.1.json
|
dont forget to offramp using https://stellar.fundable.finance/offramp its fast, free and p2p rates |
1 similar comment
|
dont forget to offramp using https://stellar.fundable.finance/offramp its fast, free and p2p rates |
…act error in slippage test
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contracts/payment-stream/src/lib.rs`:
- Around line 391-405: In the swap flow, move the destination-token balance call
represented by dest_client.balance(&contract_address) before reloading and
comparing the stream. Perform the final get_stream comparison after
balance_after is obtained, and before deriving amount_out or persisting any
state, so reentrancy during the token balance call is rejected rather than
overwritten. Add a regression test using a destination-token contract whose
balance method reenters and mutates the same stream.
🪄 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: 882878b3-5c29-45ed-8413-2a58699df793
📒 Files selected for processing (3)
contracts/payment-stream/src/lib.rscontracts/payment-stream/src/test.rscontracts/payment-stream/test_snapshots/test/test/test_deposit_with_swap_slippage_exceeded_by_contract_check.1.json
| // Re-read the stream immediately after the external swap-provider | ||
| // call and before deriving amount_out: if the provider reentered | ||
| // this contract (e.g. via withdraw, cancel_stream, pause_stream, or | ||
| // another deposit on the same stream_id), the full snapshot we | ||
| // captured before the call is stale. Reject rather than risk | ||
| // undercounting the swap output or blindly overwriting the | ||
| // reentrant state. | ||
| let mut stream: Stream = Self::get_stream(env.clone(), stream_id); | ||
| if stream != pre_swap_stream { | ||
| panic_with_error!(&env, Error::ReentrantStateChange); | ||
| } | ||
|
|
||
| let balance_after = dest_client.balance(&contract_address); | ||
| let amount_out = balance_after.checked_sub(balance_before) | ||
| .unwrap_or_else(|| panic_with_error!(&env, Error::ArithmeticOverflow)); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Move the reentrancy check after the final destination-token call.
Line 403 invokes dest_client.balance after the snapshot comparison. stream.token can be an arbitrary contract address. If that contract reenters and mutates stream_id, Lines 418-420 persist the stale stream and overwrite the reentrant update.
Read balance_after first. Then reload and compare the stream before deriving amount_out or persisting state. Add a regression test with a destination-token contract that reenters from balance.
Proposed fix
- let mut stream: Stream = Self::get_stream(env.clone(), stream_id);
- if stream != pre_swap_stream {
- panic_with_error!(&env, Error::ReentrantStateChange);
- }
-
let balance_after = dest_client.balance(&contract_address);
+ let mut stream: Stream = Self::get_stream(env.clone(), stream_id);
+ if stream != pre_swap_stream {
+ panic_with_error!(&env, Error::ReentrantStateChange);
+ }
+
let amount_out = balance_after.checked_sub(balance_before)📝 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.
| // Re-read the stream immediately after the external swap-provider | |
| // call and before deriving amount_out: if the provider reentered | |
| // this contract (e.g. via withdraw, cancel_stream, pause_stream, or | |
| // another deposit on the same stream_id), the full snapshot we | |
| // captured before the call is stale. Reject rather than risk | |
| // undercounting the swap output or blindly overwriting the | |
| // reentrant state. | |
| let mut stream: Stream = Self::get_stream(env.clone(), stream_id); | |
| if stream != pre_swap_stream { | |
| panic_with_error!(&env, Error::ReentrantStateChange); | |
| } | |
| let balance_after = dest_client.balance(&contract_address); | |
| let amount_out = balance_after.checked_sub(balance_before) | |
| .unwrap_or_else(|| panic_with_error!(&env, Error::ArithmeticOverflow)); | |
| // Re-read the stream immediately after the external swap-provider | |
| // call and before deriving amount_out: if the provider reentered | |
| // this contract (e.g. via withdraw, cancel_stream, pause_stream, or | |
| // another deposit on the same stream_id), the full snapshot we | |
| // captured before the call is stale. Reject rather than risk | |
| // undercounting the swap output or blindly overwriting the | |
| // reentrant state. | |
| let balance_after = dest_client.balance(&contract_address); | |
| let mut stream: Stream = Self::get_stream(env.clone(), stream_id); | |
| if stream != pre_swap_stream { | |
| panic_with_error!(&env, Error::ReentrantStateChange); | |
| } | |
| let amount_out = balance_after.checked_sub(balance_before) | |
| .unwrap_or_else(|| panic_with_error!(&env, Error::ArithmeticOverflow)); |
🤖 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 `@contracts/payment-stream/src/lib.rs` around lines 391 - 405, In the swap
flow, move the destination-token balance call represented by
dest_client.balance(&contract_address) before reloading and comparing the
stream. Perform the final get_stream comparison after balance_after is obtained,
and before deriving amount_out or persisting any state, so reentrancy during the
token balance call is rejected rather than overwritten. Add a regression test
using a destination-token contract whose balance method reenters and mutates the
same stream.
…-deposit # Conflicts: # contracts/payment-stream/src/lib.rs # contracts/payment-stream/src/test.rs
…-deposit # Conflicts: # contracts/payment-stream/src/lib.rs # contracts/payment-stream/src/test.rs
2 participants
Summary
deposit_with_swapto the payment-stream contract, allowing a stream sender to deposit by atomically converting a different source asset (e.g. XLM) into the stream's token (e.g. USDC) through a configured swap provider contract (a Stellar DEX/AMM router such as Soroswap), instead of depositing the stream token directly.set_swap_provider(admin-gated) andget_swap_providerto configure the router address used for conversions.SwapProviderNotSet,InvalidSwapPath,SlippageExceeded.SwapProvidercross-contract interface (#[contractclient]) that any conforming DEX/AMM router contract implements.Design notes
Soroban contracts cannot directly invoke classic Stellar DEX path-payment operations; those are top-level transaction operations, not contract-callable. The standard on-chain pattern for atomic swaps from within a contract is to call out to another contract (an AMM/router) that performs the conversion.
deposit_with_swapescrows the source asset to the configured swap provider, invokes itsswapfunction, and measures the actual destination-token balance delta received before crediting the stream (rather than trusting the provider's return value), enforcing slippage and deposit-cap checks along the way.Test plan
cargo test -p payment-stream— 48/48 tests pass, including 9 new tests covering success, non-1:1 exchange rates, provider-not-set, same-asset rejection, slippage, deposit-cap overflow, inactive-stream rejection, and admin-auth enforcement onset_swap_provider.cargo build -p payment-stream --target wasm32v1-none --release— compiles cleanly with zero warnings.Closes #519
Summary by CodeRabbit
New Features
Bug Fixes
Tests