Feat/payment vault events #11
Conversation
📝 WalkthroughWalkthroughThe PR implements oracle-based session finalization and booking lifecycle management. Changes introduce token transfer logic for booking creation and settlement, new storage structures for booking state, error handling for validation failures, and event emission for finalized sessions. Changes
Sequence DiagramssequenceDiagram
participant User
participant Contract as Payment Vault
participant Token as Token Contract
User->>Contract: create_booking(user, expert, rate, duration)
activate Contract
Contract->>Contract: Validate rate & compute total_deposit
Contract->>Token: transfer_from(user, contract, total_deposit)
activate Token
Token-->>Contract: ✓ transfer confirmed
deactivate Token
Contract->>Contract: Create & persist Booking (Pending)
Contract->>Contract: Increment & store BookingCounter
Contract-->>User: booking_id
deactivate Contract
sequenceDiagram
participant Oracle
participant Contract as Payment Vault
participant Token as Token Contract
participant Expert
participant User
Oracle->>Contract: finalize_session(booking_id, actual_duration)
activate Contract
Contract->>Contract: oracle.require_auth()
Contract->>Contract: Load & validate Booking (Pending)
Contract->>Contract: Compute expert_pay & refund
Contract->>Token: transfer(expert, expert_pay)
activate Token
Token->>Expert: deposit expert_pay
Token-->>Contract: ✓ transfer confirmed
deactivate Token
alt refund > 0
Contract->>Token: transfer(user, refund)
activate Token
Token->>User: deposit refund
Token-->>Contract: ✓ transfer confirmed
deactivate Token
end
Contract->>Contract: Update Booking status to Complete
Contract->>Contract: Emit SessionFinalized event
Contract-->>Oracle: ✓ success
deactivate Contract
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 2
🤖 Fix all issues with AI agents
In `@contracts/payment-vault-contract/src/contract.rs`:
- Around line 91-98: Add an explicit validation before computing
expert_pay/refund: check if actual_duration > booking.booked_duration and either
(a) return a clearer error variant (e.g.,
VaultError::ActualDurationExceedsBooked) instead of InvalidAmount, or (b) cap
actual_duration = booking.booked_duration (use std::cmp::min) before calculating
expert_pay and refund; ensure this check is placed prior to using
actual_duration in the expert_pay and refund calculations and update any
affected error handling or tests accordingly.
In `@contracts/payment-vault-contract/src/storage.rs`:
- Around line 64-73: get_next_booking_id currently reads and writes
DataKey::BookingCounter from instance storage (env.storage().instance()), which
can expire and lead to booking ID reuse; change it to use persistent storage
(env.storage().persistent()) when getting and setting BookingCounter so the
counter survives instance TTL and prevents ID collisions with persistent Booking
records. Locate get_next_booking_id and replace the instance().get/set calls for
DataKey::BookingCounter with persistent().get/set while preserving the logic
that increments and returns next. Ensure you reference DataKey::BookingCounter,
get_next_booking_id, and the existing Booking persistent storage semantics when
making the change.
🧹 Nitpick comments (5)
contracts/payment-vault-contract/src/storage.rs (1)
88-93: Silent no-op if booking doesn't exist.
update_booking_statussilently does nothing when the booking is not found. While the current caller (finalize_session) validates existence beforehand, this could mask bugs in future usage.Consider returning a
boolorOptionto indicate whether the update succeeded.♻️ Optional improvement
-pub fn update_booking_status(env: &Env, booking_id: u64, status: BookingStatus) { - if let Some(mut booking) = get_booking(env, booking_id) { +pub fn update_booking_status(env: &Env, booking_id: u64, status: BookingStatus) -> bool { + if let Some(mut booking) = get_booking(env, booking_id) { booking.status = status; save_booking(env, &booking); + true + } else { + false } }contracts/payment-vault-contract/src/contract.rs (2)
40-45: Potential arithmetic overflow in deposit calculation.The multiplication
rate * (booked_duration as i128)on line 41 can overflow for large values. Whilei128has a large range, extremely high rates or durations could cause unexpected behavior.The subsequent
total_deposit <= 0check partially guards against this (signed overflow wraps), but explicit overflow handling would be clearer.♻️ Suggested safer calculation
// Calculate total deposit - let total_deposit = rate * (booked_duration as i128); - - if total_deposit <= 0 { - return Err(VaultError::InvalidAmount); - } + let total_deposit = rate.checked_mul(booked_duration as i128) + .filter(|&d| d > 0) + .ok_or(VaultError::InvalidAmount)?;
92-93: Same arithmetic overflow concern in finalization.
expert_pay = booking.rate * (actual_duration as i128)has the same overflow risk. Consider usingchecked_mulfor consistency with the suggested improvement increate_booking.♻️ Safer calculation
// 4. Calculate payments - let expert_pay = booking.rate * (actual_duration as i128); - let refund = booking.total_deposit - expert_pay; - - // Ensure calculations are valid - if expert_pay < 0 || refund < 0 { - return Err(VaultError::InvalidAmount); - } + let expert_pay = booking.rate + .checked_mul(actual_duration as i128) + .ok_or(VaultError::InvalidAmount)?; + let refund = booking.total_deposit + .checked_sub(expert_pay) + .filter(|&r| r >= 0) + .ok_or(VaultError::InvalidAmount)?;contracts/payment-vault-contract/src/test.rs (2)
8-8: Unusedextern crate stdimport.This line appears unused in the current tests. If it was added for debugging (e.g.,
println!), consider removing it before merging.♻️ Remove if unused
-extern crate std;
207-221: Consider adding tests for additional edge cases.The test suite covers core scenarios well. Consider adding tests for:
create_bookingwithrate <= 0(should returnInvalidAmount)create_bookingwithbooked_duration = 0(should returnInvalidAmount)finalize_sessionwithactual_duration > booked_duration(currently returnsInvalidAmount)These would strengthen validation coverage.
💡 Example test for invalid rate
#[test] fn test_invalid_rate_rejected() { let env = Env::default(); env.mock_all_auths(); let admin = Address::generate(&env); let user = Address::generate(&env); let expert = Address::generate(&env); let oracle = Address::generate(&env); let token_admin = Address::generate(&env); let token = create_token_contract(&env, &token_admin); token.mint(&user, &10_000); let client = create_client(&env); client.init(&admin, &token.address, &oracle); // Rate of 0 should fail let result = client.try_create_booking(&user, &expert, &0, &100); assert!(result.is_err()); }
| // 4. Calculate payments | ||
| let expert_pay = booking.rate * (actual_duration as i128); | ||
| let refund = booking.total_deposit - expert_pay; | ||
|
|
||
| // Ensure calculations are valid | ||
| if expert_pay < 0 || refund < 0 { | ||
| return Err(VaultError::InvalidAmount); | ||
| } |
There was a problem hiding this comment.
Missing validation: actual_duration can exceed booked_duration.
The Oracle can pass an actual_duration greater than booked_duration, which would make refund negative (line 93) and trigger InvalidAmount. While this prevents overpayment, the error is misleading.
Consider adding explicit validation to reject actual_duration > booked_duration with a clearer error, or cap actual_duration at booked_duration.
🔧 Option 1: Explicit validation
+ // Ensure actual duration doesn't exceed booked duration
+ if actual_duration > booking.booked_duration {
+ return Err(VaultError::InvalidAmount);
+ }
+
// 4. Calculate payments
let expert_pay = booking.rate * (actual_duration as i128);
let refund = booking.total_deposit - expert_pay;
-
- // Ensure calculations are valid
- if expert_pay < 0 || refund < 0 {
- return Err(VaultError::InvalidAmount);
- }🔧 Option 2: Cap duration
+ let capped_duration = actual_duration.min(booking.booked_duration);
+
// 4. Calculate payments
- let expert_pay = booking.rate * (actual_duration as i128);
+ let expert_pay = booking.rate * (capped_duration as i128);
let refund = booking.total_deposit - expert_pay;📝 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.
| // 4. Calculate payments | |
| let expert_pay = booking.rate * (actual_duration as i128); | |
| let refund = booking.total_deposit - expert_pay; | |
| // Ensure calculations are valid | |
| if expert_pay < 0 || refund < 0 { | |
| return Err(VaultError::InvalidAmount); | |
| } | |
| // 4. Calculate payments | |
| let capped_duration = actual_duration.min(booking.booked_duration); | |
| let expert_pay = booking.rate * (capped_duration as i128); | |
| let refund = booking.total_deposit - expert_pay; | |
| // Ensure calculations are valid | |
| if expert_pay < 0 || refund < 0 { | |
| return Err(VaultError::InvalidAmount); | |
| } |
🤖 Prompt for AI Agents
In `@contracts/payment-vault-contract/src/contract.rs` around lines 91 - 98, Add
an explicit validation before computing expert_pay/refund: check if
actual_duration > booking.booked_duration and either (a) return a clearer error
variant (e.g., VaultError::ActualDurationExceedsBooked) instead of
InvalidAmount, or (b) cap actual_duration = booking.booked_duration (use
std::cmp::min) before calculating expert_pay and refund; ensure this check is
placed prior to using actual_duration in the expert_pay and refund calculations
and update any affected error handling or tests accordingly.
| pub fn get_next_booking_id(env: &Env) -> u64 { | ||
| let current: u64 = env | ||
| .storage() | ||
| .instance() | ||
| .get(&DataKey::BookingCounter) | ||
| .unwrap_or(0); | ||
| let next = current + 1; | ||
| env.storage().instance().set(&DataKey::BookingCounter, &next); | ||
| next | ||
| } |
There was a problem hiding this comment.
Storage type mismatch could cause booking ID reuse.
BookingCounter uses instance storage (which has TTL and can expire), while Booking records use persistent storage. If the instance storage expires and is later recreated, the counter resets to 0, potentially causing ID collisions with existing persistent bookings.
Consider using persistent storage for BookingCounter to ensure IDs remain unique across the contract's lifetime.
🔧 Suggested fix
pub fn get_next_booking_id(env: &Env) -> u64 {
let current: u64 = env
.storage()
- .instance()
+ .persistent()
.get(&DataKey::BookingCounter)
.unwrap_or(0);
let next = current + 1;
- env.storage().instance().set(&DataKey::BookingCounter, &next);
+ env.storage().persistent().set(&DataKey::BookingCounter, &next);
next
}📝 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.
| pub fn get_next_booking_id(env: &Env) -> u64 { | |
| let current: u64 = env | |
| .storage() | |
| .instance() | |
| .get(&DataKey::BookingCounter) | |
| .unwrap_or(0); | |
| let next = current + 1; | |
| env.storage().instance().set(&DataKey::BookingCounter, &next); | |
| next | |
| } | |
| pub fn get_next_booking_id(env: &Env) -> u64 { | |
| let current: u64 = env | |
| .storage() | |
| .persistent() | |
| .get(&DataKey::BookingCounter) | |
| .unwrap_or(0); | |
| let next = current + 1; | |
| env.storage().persistent().set(&DataKey::BookingCounter, &next); | |
| next | |
| } |
🤖 Prompt for AI Agents
In `@contracts/payment-vault-contract/src/storage.rs` around lines 64 - 73,
get_next_booking_id currently reads and writes DataKey::BookingCounter from
instance storage (env.storage().instance()), which can expire and lead to
booking ID reuse; change it to use persistent storage
(env.storage().persistent()) when getting and setting BookingCounter so the
counter survives instance TTL and prevents ID collisions with persistent Booking
records. Locate get_next_booking_id and replace the instance().get/set calls for
DataKey::BookingCounter with persistent().get/set while preserving the logic
that increments and returns next. Ensure you reference DataKey::BookingCounter,
get_next_booking_id, and the existing Booking persistent storage semantics when
making the change.
2 participants
🧠 SkillSphere Pull Request 🌐
Mark with an
xall the checkboxes that apply (like[x])cargo test(All tests passed)📌 Type of Change
📝 Changes Description
Implemented the Oracle Settlement (Finalize) feature for the Payment Vault Contract, enabling trustless, backend-verified session settlements with automatic payment distribution and refunds.
This feature allows a trusted Oracle (backend) to finalize consultation sessions by providing the actual session duration, ensuring correct expert payouts and user refunds in a fully on-chain, auditable manner.
🔑 Key Changes
1️⃣ Booking System (
storage.rs)BookingStatusenum (Pending,Complete) for lifecycle managementBookingstruct with:id,expert,userrate,total_depositbooked_durationstatussave_booking()get_booking()update_booking_status()2️⃣ Core Contract Logic (
contract.rs)create_booking()total_deposit = rate × booked_durationfinalize_session()(Oracle-only)oracle.require_auth()(critical security check)Pendingstatusexpert_pay = rate × actual_durationrefund = total_deposit - expert_payComplete(prevents double finalization)SessionFinalizedevent3️⃣ Error Handling (
error.rs)BookingNotFoundBookingNotPendingInvalidAmount4️⃣ Events (
events.rs)SessionFinalizedevent for off-chain indexing and monitoring5️⃣ Public API (
lib.rs)create_booking()andfinalize_session()with inline documentation🗄️ Storage Impact
BookingCounterBookingrecords📸 Evidence
✅ Test Results (7 / 7 Passed)
🧪 Covered Scenarios
🏗️ Build Status
wasm32-unknown-unknownrelease🌌 Comments
🔐 Security Considerations
finalize_session()enforcesoracle.require_auth()to ensure only the trusted backend can trigger settlements.🔍 Review Focus Areas
🚀 Next Steps
Thank you for contributing to SkillSphere! 🌍
Together, we’re building a trustless, peer-to-peer consulting economy on Stellar. 🚀
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.