feat(payment-vault): User Booking Cancellation + Dynamic Precision Handling#53
Merged
Bosun-Josh121 merged 1 commit intoLightForgeHub:mainfrom Mar 27, 2026
Conversation
…andling Implements issues LightForgeHub#36 and LightForgeHub#38 for the payment-vault-contract. Issue LightForgeHub#36 — User Booking Cancellation: - Add `Cancelled = 5` variant to `BookingStatus` - Add `started_at: Option<u64>` to `BookingRecord` to track when Oracle marks a session active - Add `mark_session_started()` (Oracle-only) to set started_at on a booking - Add `cancel_booking()` (User-only) — refunds full deposit when session is Pending and not yet started - Add `SessionAlreadyStarted` and `Overflow` variants to `VaultError` - Add `booking_cancelled` and `session_started` events - Add `update_booking_started_at()` to storage layer - Tests: cancel before start (success), cancel after Oracle marks started (fails) Issue LightForgeHub#38 — Dynamic Precision Handling: - Use `checked_mul` in `book_session` and `finalize_session` to prevent i128 overflow on high-precision tokens - Document that `rate_per_second` must always be in atomic units of the payment token - Test: simulate booking with 18-decimal scale (10^18 rate) to confirm no overflow Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
📝 WalkthroughWalkthroughThis PR extends the payment vault contract with user-initiated booking cancellation and oracle-driven session start tracking. It adds overflow protection to monetary calculations, introduces a new Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Contract
participant Storage
participant TokenClient
participant EventLog
User->>Contract: cancel_booking(booking_id)
Contract->>Storage: load booking
Storage-->>Contract: BookingRecord{status: Pending, started_at: None}
Contract->>TokenClient: transfer(user, total_deposit)
TokenClient-->>Contract: ✓
Contract->>Storage: update_booking_status(Cancelled)
Contract->>EventLog: booking_cancelled(booking_id, amount)
EventLog-->>Contract: ✓
Contract-->>User: Ok(())
sequenceDiagram
actor Oracle
participant Contract
participant Storage
participant EventLog
Oracle->>Contract: mark_session_started(booking_id)
Contract->>Storage: load booking
Storage-->>Contract: BookingRecord{status: Pending, started_at: None}
Contract->>Storage: update_booking_started_at(current_timestamp)
Storage-->>Contract: ✓
Contract->>EventLog: session_started(booking_id, timestamp)
EventLog-->>Contract: ✓
Contract-->>Oracle: Ok(())
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 1
🧹 Nitpick comments (1)
contracts/payment-vault-contract/src/test.rs (1)
1331-1370: The new overflow handling still lacks a failing-case regression.This validates a large valid amount, but it never forces the new
checked_mulerror path. A targeted test that exceedsi128inbook_sessionandfinalize_sessionwould lock inVaultError::Overflow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/payment-vault-contract/src/test.rs` around lines 1331 - 1370, The test validates a large-but-valid deposit and never hits the new checked_mul overflow path; add a failing-case regression that forces the overflow and asserts VaultError::Overflow from book_session and/or finalize_session. Create a new test (e.g., test_booking_overflow_triggers_vaulterror) that computes a rate_per_second and/or duration so rate_per_second.checked_mul(duration as i128) returns None (for example rate = i128::MAX / duration as i128 + 1 or use duration large enough), mint any required tokens, call client.book_session (or call finalize_session if overflow happens there), and assert the call returns the VaultError::Overflow variant; reference the functions book_session, finalize_session and the error VaultError::Overflow when locating where to trigger and assert the overflow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/payment-vault-contract/src/contract.rs`:
- Around line 252-260: The booking is stamped with started_at but left in
BookingStatus::Pending, which allows reclaim_stale_session and reject_session to
operate on started sessions and permits overwriting started_at; update the code
in this path (around storage::get_booking, storage::update_booking_started_at,
events::session_started) to also transition the booking status out of Pending
(e.g., set to a Started/Active enum variant) via the appropriate storage update,
and emit any corresponding event, or alternatively modify reclaim_stale_session
and reject_session to first check booking.started_at.is_none() before acting;
pick one: either change this flow to call storage::update_booking_status(...) to
mark the booking started when stamping started_at, or add started_at.is_none()
guards in reclaim_stale_session/reject_session.
---
Nitpick comments:
In `@contracts/payment-vault-contract/src/test.rs`:
- Around line 1331-1370: The test validates a large-but-valid deposit and never
hits the new checked_mul overflow path; add a failing-case regression that
forces the overflow and asserts VaultError::Overflow from book_session and/or
finalize_session. Create a new test (e.g.,
test_booking_overflow_triggers_vaulterror) that computes a rate_per_second
and/or duration so rate_per_second.checked_mul(duration as i128) returns None
(for example rate = i128::MAX / duration as i128 + 1 or use duration large
enough), mint any required tokens, call client.book_session (or call
finalize_session if overflow happens there), and assert the call returns the
VaultError::Overflow variant; reference the functions book_session,
finalize_session and the error VaultError::Overflow when locating where to
trigger and assert the overflow.
🪄 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
Run ID: c3bb98a4-1f40-4ff0-9711-724ad5ada36d
📒 Files selected for processing (7)
contracts/payment-vault-contract/src/contract.rscontracts/payment-vault-contract/src/error.rscontracts/payment-vault-contract/src/events.rscontracts/payment-vault-contract/src/lib.rscontracts/payment-vault-contract/src/storage.rscontracts/payment-vault-contract/src/test.rscontracts/payment-vault-contract/src/types.rs
Comment on lines
+252
to
+260
| let booking = storage::get_booking(env, booking_id).ok_or(VaultError::BookingNotFound)?; | ||
|
|
||
| if booking.status != BookingStatus::Pending { | ||
| return Err(VaultError::BookingNotPending); | ||
| } | ||
|
|
||
| let started_at = env.ledger().timestamp(); | ||
| storage::update_booking_started_at(env, booking_id, started_at); | ||
| events::session_started(env, booking_id, started_at); |
There was a problem hiding this comment.
A started booking still remains on the pre-start lifecycle path.
This only stamps started_at; it leaves the record Pending. reclaim_stale_session and reject_session still accept Pending bookings, so a started session can still be fully unwound later, and repeated calls here can overwrite the original start timestamp. Either move the booking out of Pending here or gate those paths on started_at.is_none().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/payment-vault-contract/src/contract.rs` around lines 252 - 260, The
booking is stamped with started_at but left in BookingStatus::Pending, which
allows reclaim_stale_session and reject_session to operate on started sessions
and permits overwriting started_at; update the code in this path (around
storage::get_booking, storage::update_booking_started_at,
events::session_started) to also transition the booking status out of Pending
(e.g., set to a Started/Active enum variant) via the appropriate storage update,
and emit any corresponding event, or alternatively modify reclaim_stale_session
and reject_session to first check booking.started_at.is_none() before acting;
pick one: either change this flow to call storage::update_booking_status(...) to
mark the booking started when stamping started_at, or add started_at.is_none()
guards in reclaim_stale_session/reject_session.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements issues #36 and #38 for the
payment-vault-contract.Issue #36 — User Booking Cancellation
Cancelled = 5toBookingStatusstarted_at: Option<u64>toBookingRecord— set by Oracle when session becomes activemark_session_started(booking_id)function (Oracle-only): locks the booking against user cancellationcancel_booking(user, booking_id)function (User-only): refunds full deposit when booking is Pending and Oracle hasn't started it yetSessionAlreadyStarted = 11,Overflow = 12booking_cancelled,session_startedupdate_booking_started_atIssue #38 — Dynamic Precision Handling
*withchecked_mulinbook_sessionandfinalize_sessionto prevent i128 overflow with high-precision tokens (e.g., 18-decimal ERC20 equivalents)rate_per_secondandtotal_depositmust be expressed in atomic units of the payment tokenVaultError::Overflowon multiplication overflowTest plan
test_user_cancels_before_session_starts_success— user cancels immediately, full refund received, status isCancelledtest_user_cannot_cancel_after_oracle_marks_started— Oracle marks session started, user cancel attempt fails, funds remain lockedtest_booking_with_18_decimal_token_scale_no_overflow— rate of 10^18 atomic units/sec × 100 seconds = 10^20 deposit; no overflow, finalization and refund work correctlyCloses #36
Closes #38
Summary by CodeRabbit
New Features
Improvements