Add contract-level stream expiry and auto-complete logic - #670
Add contract-level stream expiry and auto-complete logic#670jerrymusaga wants to merge 1 commit into
Conversation
Adds a `completed` flag to Stream and a StreamCompleted event so streams transition to a completed state automatically once now >= end_time, matching the existing scheduled/active/paused/completed/canceled status model used elsewhere in the app. - get_stream() derives completed status from the current ledger time (excluding canceled streams, which remain a distinct terminal state). - claim() persists the completed flag and emits StreamCompleted the first time a claim is processed at or after end_time. - claimable() already caps vesting at end_time, so no further claims are accepted beyond the fully vested amount once a stream is completed. - Adds boundary tests at exactly end_time and one second after, plus coverage for the StreamCompleted event and cancel/complete exclusivity. Closes ritik4ever#592
|
@jerrymusaga is attempting to deploy a commit to the ritik4ever's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
@jerrymusaga 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 contract adds a persisted ChangesStream completion lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Claimant
participant StreamContract
participant Ledger
participant EventObserver
Claimant->>StreamContract: claim(stream_id)
StreamContract->>Ledger: read current timestamp
Ledger-->>StreamContract: timestamp at or after end_time
StreamContract->>StreamContract: persist completed=true
StreamContract->>EventObserver: emit StreamClaimed and StreamCompleted
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
Actionable comments posted: 3
🤖 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/src/lib.rs`:
- Around line 376-379: Update the completion checks in the stream-loading logic
and the claim flow to compare against the effective vesting timestamp that
accounts for paused duration, rather than the raw ledger timestamp versus the
original end_time. Reuse the existing paused-time/vesting calculation and ensure
completion remains false while paused, then becomes true only when the effective
timestamp reaches the extended end_time.
- Around line 464-466: Make persisted completion terminal in the stream claim
and cancellation logic in contracts/src/lib.rs:464-466: reject stored completed
streams in claim, ensure the terminal claim withdraws the full remaining
claimable balance before persisting completed, and reject cancellation of
completed streams. In contracts/src/test.rs:2443-2466, add a partial
terminal-claim scenario and verify a later claim is rejected for completion
rather than zero balance; in contracts/src/test.rs:2469-2494, add a
completed-then-cancelled scenario and verify cancellation is rejected.
- Line 28: Update the persisted Stream contracttype schema and DataKey::Stream
access path to preserve decoding of records written before completed was added.
Implement an explicit migration or versioned schema that reads the legacy Stream
format, supplies the completed value, and writes the new format before normal
access; do not deploy the incompatible field addition without this compatibility
path.
🪄 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: b6819c9b-03de-4870-bb50-002cdfa18c40
📒 Files selected for processing (2)
contracts/src/lib.rscontracts/src/test.rs
| pub canceled: bool, | ||
| pub paused: bool, | ||
| pub pause_started_at: Option<u64>, | ||
| pub completed: bool, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Locate manifests and inspect persisted Stream storage plus any migration support.
fd -a -t f 'Cargo.toml' .
rg -n -C3 'struct Stream|DataKey::Stream|persistent\(\).*set|persistent\(\).*get' contracts
rg -n -i -C2 'migration|migrate|upgrade|legacy|versioned' .Repository: ritik4ever/stellar-stream
Length of output: 50382
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf 'Cargo.toml manifests:\n'
fd -a -t f 'Cargo.toml' . | sed 's#^\./##'
printf '\ncontracts/src/lib.rs outline and relevant sections:\n'
wc -l contracts/src/lib.rs contracts/src/test.rs 2>/dev/null || true
ast-grep outline contracts/src/lib.rs --view expanded 2>/dev/null | sed -n '1,220p' || true
printf '\nRelevant contracts/src/lib.rs:\n'
sed -n '1,220p' contracts/src/lib.rs
printf '\nFocused storage/key references:\n'
rg -n -C3 'struct Stream|enum DataKey|DataKey::Stream|DataKey::\w|persistent\(\)[^(]+\(\w*::[A-Z][[:alpha:]]*|StorageKey::Persistent|try_from_slice|IntoVal|FromVal|completed' contracts/src/lib.rs contracts/src/test.rs 2>/dev/null || true
printf '\nFocused migration/upgrade references:\n'
rg -n -i -C2 'migration|migrate|upgrade|legacy|versioned|schema|compat|backward|pending|completed' contracts . 2>/dev/null | rg -v 'target|node_modules|dist|build|package-lock' || trueRepository: ritik4ever/stellar-stream
Length of output: 50382
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf 'Cargo manifests under tracked files:\n'
git ls-files '*Cargo.toml'
printf '\ncontracts/src/lib.rs file size and relevant sections only:\n'
wc -l contracts/src/lib.rs contracts/src/test.rs
sed -n '1,240p' contracts/src/lib.rs
printf '\nFocused contract storage/key references from tracked contract files:\n'
rg -n -C4 'struct Stream|enum DataKey|DataKey::Stream|DataKey::|StorageKey::Persistent|persistent\(|try_from_slice|IntoVal|FromVal|completed' contracts/src/lib.rs contracts/src/test.rs
printf '\nFocused migration/upgrade references in tracked repo files (except lock/package dirs):\n'
rg -n -i -C2 'migration|migrate|upgrade|legacy|versioned|schema|compat|backward|pending|completed' --glob '!target/**' --glob '!node_modules/**' --glob '!package-lock.json' .Repository: ritik4ever/stellar-stream
Length of output: 50382
🌐 Web query:
Soroban rust-sdk contracttype contract storage serialization struct order add field breaking upgrade
💡 Result:
In the Soroban Rust SDK, contract data is serialized to XDR using the #[contracttype] macro [1][2]. Because Soroban contracts upgrade by replacing the WASM bytecode while leaving existing contract storage in-place, changing data structures is a high-risk operation [3]. Adding a field to a #[contracttype] struct or changing field ordering will alter the serialized XDR format, making the new WASM binary incompatible with data already stored in the ledger [3]. If your new contract attempts to deserialize existing storage using an updated struct definition, the following issues will likely occur: 1. Deserialization Failure: If the new struct has more fields than the stored data contains, the SDK will typically panic at runtime during deserialization [3]. 2. Silent Data Corruption: If the new struct has fewer fields, or if field types are modified (e.g., changing a type's underlying representation), the SDK may silently misinterpret the stored bytes, leading to data loss or state corruption [3]. Best Practices for Upgrades and Schema Evolution: - Manual Migration: There is no automatic migration callback in Soroban [3]. You must implement a dedicated migrate() entrypoint in your new WASM code to manually transform existing storage entries to the new schema [4][3][5]. - Atomic Upgrades: To avoid state inconsistencies, consider using a dedicated upgrader contract or pausing external access to the contract before and during the migration process [4][5]. - Version Tracking: Include a version identifier in your contract instance storage (e.g., using contractmeta! or a specific AppVersion key) [4][6]. Your logic should check this version to determine if a migration is required before proceeding with normal operations [4][3]. - Append-Only Logic: If you must evolve storage, a common pattern is to avoid modifying existing fields. Instead, create new storage keys for new data or maintain legacy keys alongside new ones until the migration is complete [3]. - Rollback Strategy: Always define a rollback plan. Ensure your upgrade process leaves the contract in a state that can be restored or corrected if the new code fails [4][5]. Because storage entries persist through upgrades, you must ensure that any changes to your data schema are explicitly handled by your contract logic to prevent reading inconsistent or incompatible state [3][5].
Citations:
- 1: https://docs.rs/soroban-sdk/latest/soroban_sdk/attr.contracttype.html
- 2: https://mintlify.wiki/stellar/rs-soroban-sdk/guides/custom-types
- 3: https://github.com/PlamenTSV/plamen/blob/main/agents/skills/soroban/migration-analysis/SKILL.md
- 4: https://github.com/stellar/stellar-protocol/blob/master/ecosystem%2Fsep-0049.md
- 5: https://github.com/stellar/stellar-protocol/blob/master/ecosystem/sep-0049.md
- 6: https://github.com/stellar/stellar-dev-skill/blob/main/skills/soroban/SKILL.md
Preserve compatibility with existing persisted streams.
Stream is a persisted #[contracttype] record; adding completed changes its serialized on-chain format. Existing DataKey::Stream entries written without this field can become undecodable after upgrade, so add an explicit migration/versioned schema path before deploying this contract.
🤖 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/src/lib.rs` at line 28, Update the persisted Stream contracttype
schema and DataKey::Stream access path to preserve decoding of records written
before completed was added. Implement an explicit migration or versioned schema
that reads the legacy Stream format, supplies the completed value, and writes
the new format before normal access; do not deploy the incompatible field
addition without this compatibility path.
| let mut stream = read_stream(&env, stream_id); | ||
| if !stream.completed && !stream.canceled && env.ledger().timestamp() >= stream.end_time { | ||
| stream.completed = true; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Honor paused time when deriving completion.
A stream paused before end_time is reported completed once wall-clock time reaches its original end, although vested_amount freezes at pause_started_at and resume_stream extends end_time. Derive completion from the same effective timestamp used for vesting, and apply that rule in claim as well.
🤖 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/src/lib.rs` around lines 376 - 379, Update the completion checks in
the stream-loading logic and the claim flow to compare against the effective
vesting timestamp that accounts for paused duration, rather than the raw ledger
timestamp versus the original end_time. Reuse the existing paused-time/vesting
calculation and ensure completion remains false while paused, then becomes true
only when the effective timestamp reaches the extended end_time.
| let newly_completed = !stream.completed && !stream.canceled && now >= stream.end_time; | ||
| if newly_completed { | ||
| stream.completed = true; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Make persisted completion a terminal state.
A partial claim at end_time sets completed = true, yet a later claim still succeeds because claim never checks the flag. cancel also accepts completed streams, replacing completion with cancellation. The current panic test only fails because the balance is already zero.
contracts/src/lib.rs#L464-L466: reject stored completed streams inclaim; require the terminal claim to withdraw the full remaining claimable balance before persisting completion, and reject cancellation of completed streams.contracts/src/test.rs#L2443-L2466: add a partial terminal-claim case and assert a subsequent claim is rejected due to completion rather than zero claimable balance.contracts/src/test.rs#L2469-L2494: add a completed-then-cancelled case and assert cancellation is rejected.
📍 Affects 2 files
contracts/src/lib.rs#L464-L466(this comment)contracts/src/test.rs#L2443-L2466contracts/src/test.rs#L2469-L2494
🤖 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/src/lib.rs` around lines 464 - 466, Make persisted completion
terminal in the stream claim and cancellation logic in
contracts/src/lib.rs:464-466: reject stored completed streams in claim, ensure
the terminal claim withdraws the full remaining claimable balance before
persisting completed, and reject cancellation of completed streams. In
contracts/src/test.rs:2443-2466, add a partial terminal-claim scenario and
verify a later claim is rejected for completion rather than zero balance; in
contracts/src/test.rs:2469-2494, add a completed-then-cancelled scenario and
verify cancellation is rejected.
|
Hi @jerrymusaga, This PR could not be merged because it has merge conflicts with the target branch. Please resolve the merge conflicts, push the updated changes, and the PR can be reviewed and merged. Thank you! |
2 participants
Adds a
completedflag to Stream and a StreamCompleted event so streams transition to a completed state automatically once now >= end_time, matching the existing scheduled/active/paused/completed/canceled status model used elsewhere in the app.Closes #592
Summary by CodeRabbit
New Features
StreamCompletedevent with completion details.Bug Fixes
Tests