feat(payment-stream): support linear vesting schedule with cliff period in escrow - #609
Conversation
Add an optional cliff (lockup) period to the payment-stream escrow contract so milestone grants can release nothing until a cliff elapses, then vest linearly. - Add cliff_duration field to Stream and create_stream_with_cliff(...). - enforce the cliff in withdrawable_amount: 0 until the same elapses, then linear vesting over the full window (pro-rata cliff share available at boundary). Reject cliffs that are not shorter than the total duration. - Add InvalidCliff error code and update create_stream docs. Also fix pre-existing broken-merge compile errors on main (from Fundable-Protocol#508/Fundable-Protocol#510): - import Symbol/Vec, define DexRouter contract client, add missing InvalidSwapPath/SlippageExceeded/SwapFailed error variants. - unify emergency-pause and dex-router storage under the DataKey enum so the circuit breaker works (was reading unset symbol keys -> NotInitialized). - remove references to the non-existent require_not_paused. - fix events.events() snapshots len assertions in tests. Adds 10 cliff vesting tests (success, failure, edge cases). cargo test: 65/65. Closes Fundable-Protocol#513
|
@BernardOnuh 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! 🚀 |
📝 WalkthroughWalkthroughThe payment-stream contract now supports optional cliff periods for linear vesting. It adds typed pause and DEX-router storage keys, swap-related declarations, cliff validation, cliff-aware withdrawals, comprehensive tests, and updated documentation. ChangesPayment stream contract
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Sender
participant PaymentStream
participant StreamStorage
Sender->>PaymentStream: create_stream_with_cliff(...)
PaymentStream->>StreamStorage: store stream and cliff_duration
PaymentStream->>PaymentStream: calculate withdrawable amount
PaymentStream-->>Sender: release vested amount after cliff
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
contracts/payment-stream/src/lib.rs (2)
610-644: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
deposit_with_swapdoes not enforce the emergency pause circuit breaker.
create_stream_internal,deposit,withdraw, andwithdraw_maxall callSelf::assert_not_paused(&env)as their first check.deposit_with_swapdoes not call it anywhere in its body (lines 610-750). While the global pause flag is active, callers can still route funds into a stream throughdeposit_with_swap, defeating the purpose ofemergency_pause.The doc comment for
emergency_pause(lines 268-272) states that pausing blockscreate_stream,deposit,withdraw, andwithdraw_max, but does not mentiondeposit_with_swap, and thedeposit_with_swaperror table (lines 601-609) never listsContractPaused. This looks like an oversight rather than an intentional exclusion, sincedeposit_with_swapis functionally a deposit path.Add the guard at the top of the function, and add a corresponding test (mirroring
test_deposit_blocked_when_paused) to confirm the behavior.🛡️ Proposed fix
pub fn deposit_with_swap( env: Env, stream_id: u64, from_token: Address, amount_in: i128, min_amount_out: i128, swap_path: Vec<Address>, ) { + Self::assert_not_paused(&env); + // 1. Load stream and validate status let mut stream: Stream = Self::get_stream(env.clone(), stream_id);🤖 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 610 - 644, Add Self::assert_not_paused(&env) as the first operation in deposit_with_swap, before loading the stream or validating inputs, so the emergency pause blocks this deposit path consistently. Update the emergency_pause documentation and deposit_with_swap error table to include ContractPaused, and add a test mirroring test_deposit_blocked_when_paused that verifies paused calls are rejected.
44-60: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftAdd migration protection for
Streamstorage upgrades.
Streamis persisted with#[contracttype], so existingDataKey::Stream(stream_id)values can miss the newcliff_durationmap entry. If this contract may upgrade in place over an installed instance, add a one-time storage migration or a versioned lazy migration for legacy stream records before reading withget_stream.🤖 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 44 - 60, Add migration protection for persisted Stream records before get_stream reads them, using a one-time or versioned lazy migration that detects legacy DataKey::Stream(stream_id) values missing cliff_duration, initializes the field with the intended legacy default, and records migration completion so it is not repeated.
🧹 Nitpick comments (2)
contracts/payment-stream/src/lib.rs (2)
414-448: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a params struct for
create_stream_with_cliff/create_stream_internal.Clippy flags both
create_stream_with_cliff(line 414) andcreate_stream_internal(line 439) for having 9 arguments, exceeding its default threshold of 7. Group the creation parameters into a single struct to simplify the signature and reduce the risk of argument-order mistakes at call sites.🤖 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 414 - 448, Introduce a dedicated stream-creation parameters struct and use it for both create_stream_with_cliff and create_stream_internal, grouping sender, recipient, token, amounts, times, and cliff_duration into one value. Update all call sites and field accesses while preserving the existing creation behavior and argument values.Source: Linters/SAST tools
2-6: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNarrow the scope of
#![allow(deprecated)].This attribute suppresses deprecated-item warnings for the entire crate. A file-wide allowance can hide future deprecation warnings unrelated to the intended exception. Apply
#[allow(deprecated)]on the specific item or import that needs it instead.🤖 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 2 - 6, Restrict the deprecated-warning suppression currently applied by the crate-level #![allow(deprecated)] to only the specific import or item that requires it. Remove the file-wide attribute and annotate the relevant soroban_sdk usage with #[allow(deprecated)], leaving unrelated code subject to deprecation warnings.
🤖 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.
Outside diff comments:
In `@contracts/payment-stream/src/lib.rs`:
- Around line 610-644: Add Self::assert_not_paused(&env) as the first operation
in deposit_with_swap, before loading the stream or validating inputs, so the
emergency pause blocks this deposit path consistently. Update the
emergency_pause documentation and deposit_with_swap error table to include
ContractPaused, and add a test mirroring test_deposit_blocked_when_paused that
verifies paused calls are rejected.
- Around line 44-60: Add migration protection for persisted Stream records
before get_stream reads them, using a one-time or versioned lazy migration that
detects legacy DataKey::Stream(stream_id) values missing cliff_duration,
initializes the field with the intended legacy default, and records migration
completion so it is not repeated.
---
Nitpick comments:
In `@contracts/payment-stream/src/lib.rs`:
- Around line 414-448: Introduce a dedicated stream-creation parameters struct
and use it for both create_stream_with_cliff and create_stream_internal,
grouping sender, recipient, token, amounts, times, and cliff_duration into one
value. Update all call sites and field accesses while preserving the existing
creation behavior and argument values.
- Around line 2-6: Restrict the deprecated-warning suppression currently applied
by the crate-level #![allow(deprecated)] to only the specific import or item
that requires it. Remove the file-wide attribute and annotate the relevant
soroban_sdk usage with #[allow(deprecated)], leaving unrelated code subject to
deprecation warnings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7fb57af0-286e-4392-9408-c7de5dc784f8
📒 Files selected for processing (4)
contracts/README.mdcontracts/payment-stream/src/lib.rscontracts/payment-stream/src/test.rsdocs/contracts/payment-stream.md
2 participants
Summary
Closes #513
Adds linear vesting with an optional cliff (lockup) period to the
payment-streamescrow contract so project milestone grants can withhold all funds until a cliff elapses, then release them linearly.What changed
Contract (
contracts/payment-stream/src/lib.rs)create_stream_with_cliff(...)(shared internal helper keepscreate_streambehavior identical, cliff = 0).Streamgains acliff_durationfield, surfaced byget_stream.withdrawable_amountreturns 0 untilcliff_durationseconds afterstart_time; after the cliff, tokens vest linearly over the full[start_time, end_time]window, so the pro-rata share accrued during the cliff is claimable at the boundary (standard cliff semantics).Error::InvalidCliff(Replace any types with proper TypeScript interfaces #23) whencliff_duration >= end_time - start_time.require_auth, and TTL management on all touched storage keys.Also fixed pre-existing broken-merge compile errors on
main(from PRs #508/#510) that left the crate unbuildable (28 errors) and the emergency-pause circuit breaker non-functional:Symbol/Vec; defined theDexRouter#[contractclient]; added missingInvalidSwapPath/SlippageExceeded/SwapFailederror variants.DataKeyenum (they were reading raw symbol keys never written byinitialize, causingNotInitialized).require_not_paused(redundant withassert_not_paused).events.events().len()assertions in the circuit-breaker tests.Tests — 10 new cliff tests (creation, invalid cliff, pre-cliff block, boundary release, linear midpoints, withdraw/withdraw_max before cliff, cancel refund, pause/resume interaction).
Verification
cargo test -p payment-stream: 65 passed / 0 failedcargo build -p payment-stream --release: 0 warningscargo build -p payment-stream --target wasm32v1-none --release: builds cleanly.Summary by CodeRabbit
New Features
Documentation
Tests