feat: add 48-hour timelock delay for dispute resolution payouts - #575
feat: add 48-hour timelock delay for dispute resolution payouts#575collinsezedike wants to merge 3 commits into
Conversation
|
Warning Review limit reached
Next review available in: 15 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe payment-stream contract adds disputed stream state and a 48-hour resolution timelock. Admins can queue or cancel validated payouts. Anyone can execute queued payouts after the timelock. Tests and ledger snapshots cover the lifecycle and blocked operations. ChangesPayment stream dispute resolution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Admin
participant PaymentStream
participant Anyone
participant TokenContract
Admin->>PaymentStream: Queue dispute resolution
PaymentStream->>PaymentStream: Mark stream Disputed
Anyone->>PaymentStream: Execute after 48-hour timelock
PaymentStream->>TokenContract: Transfer recipient and sender payouts
PaymentStream->>PaymentStream: Complete stream and clear dispute
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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 |
|
@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! 🚀 |
|
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 |
…on-timelock # Conflicts: # contracts/payment-stream/src/lib.rs # contracts/payment-stream/src/test.rs
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
contracts/payment-stream/src/lib.rs (3)
1591-1605: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winBound cancellation to the timelock window.
The doc states cancellation happens "before its timelock elapses". The code allows cancellation at any time while
executed == false. The admin can therefore cancel a matured resolution, re-queue it withresolve_dispute, and repeat, which defers the payout indefinitely and weakens the 48-hour guarantee.Reject cancellation once
execute_afterhas passed, or update the doc to state that the admin can cancel at any time before execution.Also consider adding
Self::assert_not_paused(&env)for consistency withresolve_disputeandexecute_resolution. If the omission is intentional, so that the admin can unwind disputes during an incident, state that in the doc.♻️ Proposed change to enforce the cancellation window
if queued.executed { panic_with_error!(&env, Error::DisputeAlreadyExecuted); } + if env.ledger().timestamp() >= queued.execute_after { + panic_with_error!(&env, Error::TimelockNotElapsed); + }🤖 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 1591 - 1605, Update cancel_queued_resolution to reject cancellation once queued.execute_after has elapsed, while preserving cancellation before the timelock and the existing executed/not-found checks. Add Self::assert_not_paused(&env) for consistency with resolve_dispute and execute_resolution, or explicitly document the intentional paused-state behavior if cancellation must remain available during incidents.
1634-1642: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExecuted dispute records are retained indefinitely.
execute_resolutionkeeps theDispute(dispute_id)record and extends its TTL after execution (Lines 1574-1576). The record then only serves historical lookup throughget_queued_resolution, whileDisputeExecutedEventalready carries the same data. Linked issue#517asks for an efficient state footprint.Consider removing the record after execution, or stop extending its TTL so it expires naturally.
🤖 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 1634 - 1642, The execute_resolution flow should no longer retain executed Dispute(dispute_id) records indefinitely. Update execute_resolution to remove the dispute record after successful execution, or omit its TTL extension so it expires naturally, while preserving DisputeExecutedEvent emission and active-dispute cleanup behavior.
1441-1447: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBlocked operations do not report
DisputeInProgress.The doc says the
Disputedstatus blocks deposits, withdrawals, pausing, resuming, and cancellation. Onlydeposit(Line 723) anddeposit_with_swap(Line 807) raiseDisputeInProgress. The other operations fall through to pre-existing status checks and report misleading codes:
withdrawreportsInsufficientWithdrawable, becausewithdrawable_amountreturns 0 for a non-Activestream.pause_streamreportsStreamNotActive.resume_streamreportsStreamNotPaused.cancel_streamreportsStreamCannotBeCanceled.The behavior is safe, but clients cannot distinguish an active dispute from an ordinary state error. Add an explicit
Disputedcheck to each of these entry points so the contract returnsDisputeInProgress.🤖 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 1441 - 1447, Add an explicit Disputed-status check to withdraw, pause_stream, resume_stream, and cancel_stream so each returns DisputeInProgress before existing state or balance validation. Preserve the current behavior for all non-disputed statuses and keep the existing deposit checks unchanged.contracts/payment-stream/src/test.rs (5)
1894-1921: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a test at the exact timelock boundary.
execute_resolutionrejects only whenenv.ledger().timestamp() < queued.execute_after. Execution at exactlyexecute_aftermust succeed. The current tests coverDAY(rejected) and2 * DAY + 1(accepted), so they do not pin the boundary. An off-by-one change to that comparison would not fail any test.💚 Proposed additional test
#[test] fn test_execute_resolution_at_exact_timelock_succeeds() { let env = Env::default(); env.mock_all_auths(); let (_admin, _sender, _recipient, _token, contract_id, stream_id) = setup_dispute_test(&env); let client = PaymentStreamContractClient::new(&env, &contract_id); let dispute_id = client.resolve_dispute(&stream_id, &600, &400); // execute_after == 0 + 2 * DAY; execution must be allowed at that instant. env.ledger().set_timestamp(2 * DAY); client.execute_resolution(&dispute_id); let stream = client.get_stream(&stream_id); assert_eq!(stream.status, StreamStatus::Completed); }🤖 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 1894 - 1921, Add a boundary test alongside test_execute_resolution_before_timelock_fails and test_execute_resolution_after_timelock_succeeds that resolves a dispute, sets the ledger timestamp to exactly 2 * DAY (the queued execute_after value), calls execute_resolution, and asserts the stream status is StreamStatus::Completed.
2104-2127: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the remaining
resolve_disputevalidation branches.Two guards in
resolve_disputehave no test:
total_resolution <= 0: callresolve_dispute(&stream_id, &0, &0). Both amounts are non-negative, so this reaches the zero-total guard rather than the negative-amount guard.checked_addoverflow: call withi128::MAXand a positive sender amount to reachError::ArithmeticOverflowbefore the balance comparison.Both tests are short and pin guards that protect payout amounts.
🤖 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 2104 - 2127, Add tests alongside test_resolve_dispute_invalid_amounts_exceeding_balance and test_resolve_dispute_negative_amounts_rejected covering the remaining resolve_dispute guards: assert that zero sender and recipient amounts return an error for the total_resolution <= 0 path, and assert that i128::MAX with a positive sender amount returns an error for the checked_add overflow path.
1870-1891: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the
let _ = admin;artifact.Bind the value as
_adminin the destructuring pattern. The other dispute tests already use that form. This removes the trailing discard statement.♻️ Proposed refactor
- let (admin, _sender, _recipient, _token, contract_id, stream_id) = setup_dispute_test(&env); + let (_admin, _sender, _recipient, _token, contract_id, stream_id) = setup_dispute_test(&env); @@ let protocol_metrics = client.get_protocol_metrics(); assert_eq!(protocol_metrics.total_active_streams, 0); - - let _ = admin; }🤖 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 1870 - 1891, Update the setup_dispute_test destructuring to bind the unused administrator value as _admin instead of admin, then remove the trailing let _ = admin; statement. Leave the remaining test logic unchanged.
1836-1863: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReturn the client from
setup_dispute_test.The helper builds a
PaymentStreamContractClientand then discards it. Every one of the 14 dispute tests re-creates the same client fromcontract_id. Return the client to remove that duplicated line.♻️ Proposed refactor
-fn setup_dispute_test(env: &Env) -> (Address, Address, Address, Address, Address, u64) { +fn setup_dispute_test( + env: &Env, +) -> (PaymentStreamContractClient, Address, Address, Address, Address, Address, u64) { let admin = Address::generate(env); @@ - (admin, sender, recipient, token, contract_id, stream_id) + (client, admin, sender, recipient, token, contract_id, stream_id) }Each caller then becomes:
let (client, _admin, _sender, _recipient, _token, contract_id, stream_id) = setup_dispute_test(&env);🤖 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 1836 - 1863, Update setup_dispute_test to return the existing PaymentStreamContractClient alongside the current setup values, preserving the client’s initialization and stream creation. Adjust all dispute-test callers to destructure the returned client and use it directly instead of reconstructing it from contract_id.
1906-1907: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the expected error variants for dispute-resolution tests.
try_execute_resolution(&dispute_id)needsErr(Ok(Error::<variant>))assertions instead ofis_err(), so tests catch the exact failure path such asTimelockNotElapsed,DisputeAlreadyExecuted, orDisputeNotFoundrather than any error.Applies to lines 1906-1907, 1950-1951, 1962-1963, 2029-2030, 2100-2101, 2113-2114, 2125-2126, 2139-2140, 2153-2154, 2168-2169, 2182-2183, 2196-2197.
🤖 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 1906 - 1907, Update each dispute-resolution test assertion around try_execute_resolution to match the exact expected Err(Ok(Error::<variant>)) value instead of using is_err(). Use the failure variant appropriate to each scenario, including TimelockNotElapsed, DisputeAlreadyExecuted, or DisputeNotFound, while preserving the existing test setup and execution flow.
🤖 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 1561-1571: Update execute_resolution so any escrow remaining after
transferring the resolved recipient_amount and sender_amount is refunded to
stream.sender before marking the stream Completed. Ensure the total payouts,
including this residual refund, consume the full escrowed balance while
preserving the existing exact-split behavior; alternatively, enforce in
resolve_dispute that total_resolution equals the escrowed balance.
In `@contracts/payment-stream/src/test.rs`:
- Around line 2011-2014: Update the comment above the total_active_streams
assertion to state that pausing decrements the active count, while preserving
the assertion value of 0 and leaving the test logic unchanged.
- Around line 2143-2155: Update test_deposit_blocked_during_dispute to ensure
the attempted deposit is otherwise valid by creating the disputed stream with
initial balance below total_amount, or assert the specific dispute-blocking
error variant returned by try_deposit. Keep the test focused on verifying that
an available deposit is rejected because the stream is under dispute, not
because it exceeds the total.
In
`@contracts/payment-stream/test_snapshots/test/test/test_execute_resolution_twice_fails.1.json`:
- Around line 402-511: The execute_resolution flow must account for both
recipient and sender transfers when updating stream settlement state. Update the
relevant balance/withdrawn_amount logic to record the full
queue.recipient_amount plus queue.sender_amount, ensuring the resulting balance
and withdrawn_amount reflect zero remaining escrow after a 600/400 resolution.
---
Nitpick comments:
In `@contracts/payment-stream/src/lib.rs`:
- Around line 1591-1605: Update cancel_queued_resolution to reject cancellation
once queued.execute_after has elapsed, while preserving cancellation before the
timelock and the existing executed/not-found checks. Add
Self::assert_not_paused(&env) for consistency with resolve_dispute and
execute_resolution, or explicitly document the intentional paused-state behavior
if cancellation must remain available during incidents.
- Around line 1634-1642: The execute_resolution flow should no longer retain
executed Dispute(dispute_id) records indefinitely. Update execute_resolution to
remove the dispute record after successful execution, or omit its TTL extension
so it expires naturally, while preserving DisputeExecutedEvent emission and
active-dispute cleanup behavior.
- Around line 1441-1447: Add an explicit Disputed-status check to withdraw,
pause_stream, resume_stream, and cancel_stream so each returns DisputeInProgress
before existing state or balance validation. Preserve the current behavior for
all non-disputed statuses and keep the existing deposit checks unchanged.
In `@contracts/payment-stream/src/test.rs`:
- Around line 1894-1921: Add a boundary test alongside
test_execute_resolution_before_timelock_fails and
test_execute_resolution_after_timelock_succeeds that resolves a dispute, sets
the ledger timestamp to exactly 2 * DAY (the queued execute_after value), calls
execute_resolution, and asserts the stream status is StreamStatus::Completed.
- Around line 2104-2127: Add tests alongside
test_resolve_dispute_invalid_amounts_exceeding_balance and
test_resolve_dispute_negative_amounts_rejected covering the remaining
resolve_dispute guards: assert that zero sender and recipient amounts return an
error for the total_resolution <= 0 path, and assert that i128::MAX with a
positive sender amount returns an error for the checked_add overflow path.
- Around line 1870-1891: Update the setup_dispute_test destructuring to bind the
unused administrator value as _admin instead of admin, then remove the trailing
let _ = admin; statement. Leave the remaining test logic unchanged.
- Around line 1836-1863: Update setup_dispute_test to return the existing
PaymentStreamContractClient alongside the current setup values, preserving the
client’s initialization and stream creation. Adjust all dispute-test callers to
destructure the returned client and use it directly instead of reconstructing it
from contract_id.
- Around line 1906-1907: Update each dispute-resolution test assertion around
try_execute_resolution to match the exact expected Err(Ok(Error::<variant>))
value instead of using is_err(). Use the failure variant appropriate to each
scenario, including TimelockNotElapsed, DisputeAlreadyExecuted, or
DisputeNotFound, while preserving the existing test setup and execution flow.
🪄 Autofix
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: 328dbbeb-ac7c-42fa-b21c-5d4019117503
📒 Files selected for processing (18)
contracts/payment-stream/src/lib.rscontracts/payment-stream/src/test.rscontracts/payment-stream/test_snapshots/test/test/test_cancel_already_executed_resolution_fails.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_cancel_queued_resolution_restores_paused_stream.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_cancel_queued_resolution_restores_stream.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_cancel_stream_blocked_during_dispute.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_deposit_blocked_during_dispute.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_execute_nonexistent_resolution_fails.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_execute_resolution_after_timelock_succeeds.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_execute_resolution_before_timelock_fails.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_execute_resolution_twice_fails.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_pause_blocked_during_dispute.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_resolve_dispute_already_disputed_fails.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_resolve_dispute_invalid_amounts_exceeding_balance.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_resolve_dispute_negative_amounts_rejected.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_resolve_dispute_queues_and_pauses_stream.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_resolve_dispute_requires_admin_auth.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_withdraw_blocked_during_dispute.1.json
| #[test] | ||
| fn test_deposit_blocked_during_dispute() { | ||
| let env = Env::default(); | ||
| env.mock_all_auths(); | ||
|
|
||
| let (_admin, _sender, _recipient, _token, contract_id, stream_id) = setup_dispute_test(&env); | ||
| let client = PaymentStreamContractClient::new(&env, &contract_id); | ||
|
|
||
| client.resolve_dispute(&stream_id, &600, &400); | ||
|
|
||
| let result = client.try_deposit(&stream_id, &1); | ||
| assert!(result.is_err()); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The deposit test does not prove the dispute block.
setup_dispute_test creates the stream with total_amount = 1000 and initial_amount = 1000, so balance already equals total_amount. try_deposit(&stream_id, &1) therefore fails the "deposit exceeds total" check as well, exactly as test_deposit_exceeds_total at line 399 shows. The assertion passes for the wrong reason, and it would still pass if the dispute block were removed.
Create a stream with headroom, or assert the specific dispute error variant.
💚 Proposed fix
fn test_deposit_blocked_during_dispute() {
let env = Env::default();
env.mock_all_auths();
- let (_admin, _sender, _recipient, _token, contract_id, stream_id) = setup_dispute_test(&env);
- let client = PaymentStreamContractClient::new(&env, &contract_id);
+ let (_admin, sender, _recipient, token, contract_id, _stream_id) = setup_dispute_test(&env);
+ let client = PaymentStreamContractClient::new(&env, &contract_id);
+
+ // Fund a second stream that has deposit headroom, so the only reason a
+ // deposit can fail is the active dispute.
+ let token_admin = token::StellarAssetClient::new(&env, &token);
+ token_admin.mint(&sender, &1000);
+ let recipient2 = Address::generate(&env);
+ let stream_id = client.create_stream(&sender, &recipient2, &token, &1000, &500, &0, &100);
client.resolve_dispute(&stream_id, &600, &400);
let result = client.try_deposit(&stream_id, &1);
assert!(result.is_err());
}🤖 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 2143 - 2155, Update
test_deposit_blocked_during_dispute to ensure the attempted deposit is otherwise
valid by creating the disputed stream with initial balance below total_amount,
or assert the specific dispute-blocking error variant returned by try_deposit.
Keep the test focused on verifying that an available deposit is rejected because
the stream is under dispute, not because it exceeds the total.
2 participants
Summary
resolve_dispute(stream_id, recipient_amount, sender_amount)(admin-gated) records the decided outcome, moves the stream into a newDisputedstatus, and queues the payout withexecute_after = now + 48h. It does not move funds immediately.execute_resolution(dispute_id)is permissionless (the outcome and amounts were already authorized when queued; only the passage of time gates it) and pays outrecipient_amountto the recipient andsender_amountback to the sender once the timelock has elapsed, then marks the streamCompleted.cancel_queued_resolution(dispute_id)(admin-gated) reverses a queued resolution before execution, restoring the stream to its pre-dispute status (ActiveorPaused), e.g. if new evidence emerges during the delay window.get_queued_resolution/get_active_disputeviews for inspecting dispute state.Disputed, deposits, withdrawals, pausing, resuming, and cancellation are all blocked until the dispute is executed or canceled.DisputeInProgress,DisputeAlreadyQueued,DisputeNotFound,DisputeAlreadyExecuted,TimelockNotElapsed,InvalidResolutionAmounts.Design notes
This repo doesn't yet have a standalone dispute/voting subsystem, so per the issue's scope (the timelock controller itself, not a full arbitration system) the admin acts as the resolver recording an already-decided outcome, consistent with how
adminalready gates protocol-level actions elsewhere in this contract (fee rate, fee collector).resolve_disputeandexecute_resolutionare split so the queued outcome and amounts are fixed and authorized up front, and execution only requires the delay to have elapsed, matching a standard timelock-controller pattern (anyone can trigger execution once ready). This also referenced in the issue's application thread as connecting to the emergency pause switch (#510); since that hasn't landed onmainyet,execute_resolutionisn't pause-gated here — happy to wire that in once #510 merges.Test plan
cargo test -p payment-stream— 55/55 tests pass, including 19 new tests covering: queuing and stream-status transition, execution before/after the timelock, double-execution, executing a nonexistent dispute, cancellation restoringActive/Pausedstate, cancelling an already-executed dispute, admin-auth enforcement, invalid/negative/over-balance resolution amounts, already-disputed rejection, and deposit/withdraw/cancel/pause being blocked mid-dispute.cargo build -p payment-stream --target wasm32v1-none --release— compiles cleanly with zero warnings.Closes #517
Summary by CodeRabbit
New Features
Tests