fix(contracts): claimable() returns 0 after cancel and auto-settle (#591) - #655
fix(contracts): claimable() returns 0 after cancel and auto-settle (#591)#655adesuwa-tech wants to merge 2 commits into
Conversation
…itik4ever#591) After cancel() the contract now atomically finalises the stream: * Send the unclaimed vested remainder to the recipient. * Refund the unvested portion to the sender. * Mark canceled, bound end_time and total_amount to the cancel moment. This makes claimable(stream_id, at_time) return 0 for any canceled stream (per the acceptance criterion) and lets claim() panic deterministically on canceled streams because the contract no longer owes anything. Previously claimable kept returning a non-zero amount based on elapsed time which left funds stranded inside the contract. Saturating_sub is used on the cancel arithmetic for defensiveness. Tests: - Updated test_cancel_after_partial_claim_*, test_cancel_recipient_*, test_split_stream_claim_and_cancel_work_per_substream, test_clawback_after_canceled_stream_* to match the new atomic semantics. - Refactored test_claim_on_canceled_stream (no longer relies on manual claim after cancel since cancel now auto-settles). - Added test_claimable_returns_zero_after_cancel (the AC). - Added test_claim_after_cancel_panics (lock down the panic path). Closes ritik4ever#591
|
Someone is attempting to deploy a commit to the ritik4ever's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
@adesuwa-tech 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! 🚀 |
📝 WalkthroughWalkthroughCanceled streams now settle vested funds to recipients, refund unvested funds to senders, and report zero claimable amounts. Tests and snapshots cover post-cancel queries, failed claims, split streams, refunds, and clawback behavior. ChangesCanceled Stream Settlement
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Sender
participant StreamContract
participant TokenClient
participant Recipient
Sender->>StreamContract: cancel(stream_id)
StreamContract->>TokenClient: transfer vested payout
TokenClient->>Recipient: recipient receives vested amount
StreamContract->>TokenClient: transfer unvested refund
TokenClient->>Sender: sender receives refund
Sender->>StreamContract: claimable(stream_id, at_time)
StreamContract-->>Sender: return 0
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/src/lib.rs (1)
601-634: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winGuard
resume_streamagainst canceled streams.
cancel()does not clearpaused, andclaim()returns0for canceled streams, butresume_stream()still persists schedule changes when!stream.pausedis true. Addif stream.canceled { panic!("stream canceled"); }before resuming, matchingpause_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/src/lib.rs` around lines 601 - 634, Add a canceled-stream guard in resume_stream immediately after loading or validating the stream, before the paused-state check and any schedule updates: if stream.canceled is true, panic with "stream canceled". Match the existing guard behavior in pause_stream while leaving non-canceled resume handling unchanged.
🧹 Nitpick comments (1)
contracts/src/lib.rs (1)
399-427: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate canceled/vested-clamp logic between
claimable()andget_claimable_batch().The
if stream.canceled { 0 } else { vested - claimed, clamp }branch here duplicates the body ofclaimable()(lines 389-397). This is exactly the class of divergence that caused issue#591(one path was updated, the other wasn't); extracting a shared helper removes the risk of the two drifting again.♻️ Suggested extraction
+fn claimable_amount(stream: &Stream, at_time: u64) -> i128 { + if stream.canceled { + return 0; + } + let vested = vested_amount(stream, at_time); + let claimable = vested - stream.claimed_amount; + if claimable < 0 { 0 } else { claimable } +} + pub fn claimable(env: Env, stream_id: u64, at_time: u64) -> i128 { let stream = read_stream(&env, stream_id); - if stream.canceled { - return 0; - } - let vested = vested_amount(&stream, at_time); - let claimable = vested - stream.claimed_amount; - if claimable < 0 { 0 } else { claimable } + claimable_amount(&stream, at_time) } pub fn get_claimable_batch(env: Env, stream_ids: Vec<u64>, at_time: u64) -> Map<u64, i128> { ... let amount = match stream_opt { - Some(stream) => { - if stream.canceled { - 0 - } else { - let vested = vested_amount(&stream, at_time); - let claimable = vested - stream.claimed_amount; - if claimable < 0 { 0 } else { claimable } - } - } + Some(stream) => claimable_amount(&stream, at_time), None => 0, };🤖 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 399 - 427, Extract the shared canceled/vested-amount calculation from claimable() into a helper, then have both claimable() and get_claimable_batch() call it. Preserve the existing zero result for canceled streams, missing streams, and negative claimable amounts while removing the duplicated branch from get_claimable_batch().
🤖 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 500-516: Update the cancellation logic in the function containing
vested_amount, recipient_payout, and sender_refund so the persisted end-time
bound uses the effective vesting cutoff represented by vested, rather than raw
ledger timestamp now. Ensure paused-then-canceled streams retain a schedule
consistent with their frozen vested amount, while preserving the existing
start-time lower bound and normal active-stream behavior.
---
Outside diff comments:
In `@contracts/src/lib.rs`:
- Around line 601-634: Add a canceled-stream guard in resume_stream immediately
after loading or validating the stream, before the paused-state check and any
schedule updates: if stream.canceled is true, panic with "stream canceled".
Match the existing guard behavior in pause_stream while leaving non-canceled
resume handling unchanged.
---
Nitpick comments:
In `@contracts/src/lib.rs`:
- Around line 399-427: Extract the shared canceled/vested-amount calculation
from claimable() into a helper, then have both claimable() and
get_claimable_batch() call it. Preserve the existing zero result for canceled
streams, missing streams, and negative claimable amounts while removing the
duplicated branch from get_claimable_batch().
🪄 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: 537d2f02-6567-4779-8ef6-a134156c8fd7
⛔ Files ignored due to path filters (1)
contracts/src/snapshots/stellar_stream__test__stream_cancel_after_partial_claim.snapis excluded by!**/*.snap
📒 Files selected for processing (4)
contracts/src/lib.rscontracts/src/test.rscontracts/test_snapshots/test/test_claim_after_cancel_panics.1.jsoncontracts/test_snapshots/test/test_claimable_returns_zero_after_cancel.1.json
| let now = env.ledger().timestamp(); | ||
| stream.canceled = true; | ||
|
|
||
| let vested = vested_amount(&stream, now); | ||
| let sender_refund = stream.total_amount - vested; | ||
|
|
||
| // `saturating_sub` keeps `recipient_payout` non-negative if a caller has | ||
| // somehow claimed more than is vested (defensive — the regular `claim` | ||
| // path already prevents this, but cancel must still be safe). | ||
| let recipient_payout = vested.saturating_sub(stream.claimed_amount); | ||
| let sender_refund = stream.total_amount.saturating_sub(vested); | ||
|
|
||
| // Mark canceled and bound the stream's vesting schedule to the | ||
| // cancel moment so any future claimable() query is bounded by `vested`. | ||
| stream.canceled = true; | ||
| let min_end = if now > stream.start_time { now } else { stream.start_time }; | ||
| if min_end < stream.end_time { | ||
| stream.end_time = min_end; | ||
| stream.total_amount = vested; | ||
| } | ||
| stream.total_amount = vested; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
min_end uses wall-clock now, not the effective (paused) vesting cutoff — corrupts the persisted schedule and can make clawback() return a negative amount for paused-then-canceled streams.
vested correctly freezes at pause_started_at when the stream is paused (per vested_amount), so recipient_payout/sender_refund are computed correctly. But min_end is derived from the raw now, not that same effective time. Example: stream 0–1000/total 1000, paused at t=300, canceled at t=800 → vested=300 (correct), but persisted end_time=800, total_amount=300. That’s an internally inconsistent linear schedule (300 vested over 800, not over 300).
This isn’t just cosmetic: clawback() (unchanged) doesn’t check stream.canceled and recomputes vested_amount directly on the persisted stream. With the inconsistent fields above, vested_amount recomputes to 112 (not 300), so unclaimed_vested = 112 - claimed_amount(300) = -188, and clawback can return a negative actual_clawback to the caller (no transfer occurs, but the reported clawback amount is wrong).
claimable()/get_claimable_batch() are unaffected since they short-circuit on canceled before touching vested_amount, so there's no direct fund-safety issue — but the stored schedule and clawback()'s return value are incorrect for this reachable pause→cancel ordering (nothing prevents cancel while paused).
🐛 Suggested fix — bound by the effective (paused-aware) time
let now = env.ledger().timestamp();
let vested = vested_amount(&stream, now);
let recipient_payout = vested.saturating_sub(stream.claimed_amount);
let sender_refund = stream.total_amount.saturating_sub(vested);
stream.canceled = true;
- let min_end = if now > stream.start_time { now } else { stream.start_time };
+ // Keep the persisted schedule consistent with the same instant used
+ // to freeze `vested` above (pause_started_at when paused).
+ let effective_now = if stream.paused {
+ stream.pause_started_at.unwrap_or(now)
+ } else {
+ now
+ };
+ let min_end = if effective_now > stream.start_time { effective_now } else { stream.start_time };
if min_end < stream.end_time {
stream.end_time = min_end;
}
stream.total_amount = vested;📝 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.
| let now = env.ledger().timestamp(); | |
| stream.canceled = true; | |
| let vested = vested_amount(&stream, now); | |
| let sender_refund = stream.total_amount - vested; | |
| // `saturating_sub` keeps `recipient_payout` non-negative if a caller has | |
| // somehow claimed more than is vested (defensive — the regular `claim` | |
| // path already prevents this, but cancel must still be safe). | |
| let recipient_payout = vested.saturating_sub(stream.claimed_amount); | |
| let sender_refund = stream.total_amount.saturating_sub(vested); | |
| // Mark canceled and bound the stream's vesting schedule to the | |
| // cancel moment so any future claimable() query is bounded by `vested`. | |
| stream.canceled = true; | |
| let min_end = if now > stream.start_time { now } else { stream.start_time }; | |
| if min_end < stream.end_time { | |
| stream.end_time = min_end; | |
| stream.total_amount = vested; | |
| } | |
| stream.total_amount = vested; | |
| let now = env.ledger().timestamp(); | |
| let vested = vested_amount(&stream, now); | |
| // `saturating_sub` keeps `recipient_payout` non-negative if a caller has | |
| // somehow claimed more than is vested (defensive — the regular `claim` | |
| // path already prevents this, but cancel must still be safe). | |
| let recipient_payout = vested.saturating_sub(stream.claimed_amount); | |
| let sender_refund = stream.total_amount.saturating_sub(vested); | |
| // Mark canceled and bound the stream's vesting schedule to the | |
| // cancel moment so any future claimable() query is bounded by `vested`. | |
| stream.canceled = true; | |
| // Keep the persisted schedule consistent with the same instant used | |
| // to freeze `vested` above (pause_started_at when paused). | |
| let effective_now = if stream.paused { | |
| stream.pause_started_at.unwrap_or(now) | |
| } else { | |
| now | |
| }; | |
| let min_end = if effective_now > stream.start_time { effective_now } else { stream.start_time }; | |
| if min_end < stream.end_time { | |
| stream.end_time = min_end; | |
| } | |
| stream.total_amount = vested; |
🤖 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 500 - 516, Update the cancellation logic
in the function containing vested_amount, recipient_payout, and sender_refund so
the persisted end-time bound uses the effective vesting cutoff represented by
vested, rather than raw ledger timestamp now. Ensure paused-then-canceled
streams retain a schedule consistent with their frozen vested amount, while
preserving the existing start-time lower bound and normal active-stream
behavior.
|
Hi @adesuwa-tech, 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! |
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 (1)
contracts/src/test.rs (1)
2506-3171: 🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy liftTest bodies and names are mismatched, and some functions use undefined variables. This block will not compile.
This range shows systemic corruption, consistent with the unresolved merge conflicts noted in the PR objectives. Concrete evidence:
- Line 2513 opens
fn test_get_allowed_tokens_returns_initialized_list()with no body before line 2514's comment//#594— Comprehensive stream state transitions & edge-case coverageappears inside the function. The next function,test_full_lifecycle_create_claim_complete(line 2519), has a body (lines 2520-2536) that actually exercisesget_allowed_tokens, not a lifecycle claim flow.test_add_allowed_token_appends_to_list(line 2551) never callsadd_allowed_token; its body (lines 2552-2578) creates a stream and claims against it, and it referencessender,recipient,admin,env, andclientwithout declaring any of them in this function.test_add_allowed_token_non_admin_panics(line 2622,#[should_panic(expected = "unauthorized")]) has a body (lines 2623-2648) that cancels and claims a stream; nothing in it raises an "unauthorized" panic.- Line 2659:
j let attacker = Address::generate(&env);has a strayjcharacter beforelet, which is a syntax error.test_over_claim_after_partial_claim(line 3005,#[should_panic(expected = "amount exceeds claimable")]) has a body (lines 3006-3021) that callsset_admin/add_allowed_tokenand its own comment states "without panicking," contradicting the#[should_panic]attribute.test_create_stream_with_allowlisted_token_succeeds(line 2792),test_create_stream_with_non_allowlisted_token_panics(line 2832),test_create_stream_rejected_after_token_removed_from_allowlist(line 2871),test_create_stream_succeeds_after_token_added_to_allowlist(line 2914),test_set_admin_transfers_admin_role(line 2964),test_set_admin_old_admin_loses_privileges(line 3027),test_set_admin_non_admin_panics(line 3063), andtest_set_admin_chain_transfer(line 3114) all referenceenv,admin,sender,recipient, orclientwith no correspondinglet env = Env::default();/registration/Address::generateboilerplate anywhere in the function body. These are self-contained scoping errors.Reconstruct this section from a correct merge resolution rather than patching individual lines; the doc comments,
#[should_panic]attributes, and bodies need to be re-paired correctly, and every test needs its own setup boilerplate.#!/bin/bash # Description: Spot-check functions flagged as missing setup boilerplate or as mismatched with their names/panics. sed -n '2506,2537p;2551,2663p;2790,2930p;2960,3141p' contracts/src/test.rsAs per the PR objectives, "The conflicts must be resolved and the updated changes pushed before review and merge," and this block appears to reflect that unresolved conflict.
🤖 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/test.rs` around lines 2506 - 3171, Reconstruct the corrupted test section from the intended merge result rather than applying isolated edits: pair each doc comment and #[should_panic] attribute with its matching test body, restore the expected behavior for symbols such as test_get_allowed_tokens_returns_initialized_list, test_add_allowed_token_appends_to_list, test_over_claim_after_partial_claim, and the allowlist/admin tests, remove the stray character before attacker, and give every test its own Env, contract/client, and address setup so no variables are out of scope and the section compiles.
🤖 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/src/test.rs`:
- Around line 2506-3171: Reconstruct the corrupted test section from the
intended merge result rather than applying isolated edits: pair each doc comment
and #[should_panic] attribute with its matching test body, restore the expected
behavior for symbols such as test_get_allowed_tokens_returns_initialized_list,
test_add_allowed_token_appends_to_list, test_over_claim_after_partial_claim, and
the allowlist/admin tests, remove the stray character before attacker, and give
every test its own Env, contract/client, and address setup so no variables are
out of scope and the section compiles.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a4f3698a-7c97-49ee-9b94-1258bfc19f29
📒 Files selected for processing (2)
contracts/src/lib.rscontracts/src/test.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/src/lib.rs
2 participants
Closes #591
After cancel() the contract atomically settles the stream: the unclaimed vested remainder is transferred to the recipient, the unvested portion is refunded to the sender, and the stream is marked canceled with end_time/total_amount bounded to the cancel moment. claimable() and get_claimable_batch() short-circuit to 0 for canceled streams. saturating_sub keeps the cancel arithmetic defensive. The new test_claimable_returns_zero_after_cancel covers the acceptance criterion. Updated existing tests assert the new atomic semantics; idempotent double-cancel still works.
Summary by CodeRabbit
Bug Fixes
Tests