Expand test coverage for escrow splits, postponement, and resale edge cases - #155
Conversation
…ale royalty edge cases Covers co-host flagging mid-event before settlement, multi-token revenue isolation during splits, escrow release after repeated admin delay extensions, the choice_deadline_ledger boundary for postponement refunds, and resale royalty configurations that exceed total proceeds.
|
@Depo-dev 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! 🚀 |
|
Warning Review limit reached
Next review available in: 41 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 (1)
📝 WalkthroughWalkthroughAdds integration and unit-test coverage for postponement refund boundaries, excessive resale royalties, co-host flagging, multi-token settlement, and cumulative withdrawal-delay extensions. ChangesContract edge-case coverage
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
contracts/payments/src/revenue_split_test.rs (1)
752-802: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd an exact-boundary assertion at the computed unlock ledger (26280).
The test proves extensions accumulate (20_000 fails, 26_281 succeeds) but never checks ledger 26_280 itself, so an off-by-one in the cumulative extension math wouldn't be caught. The sibling postponement tests in
contracts/event/src/integration_tests.rstest both "exactly at deadline" and "one past deadline" — worth mirroring that precision here.♻️ Suggested addition
env.ledger().with_mut(|li| li.sequence_number = 20_000); assert_eq!( client.try_withdraw_split(&cohost, &event_id).err(), Some(Ok(PaymentError::EscrowNotExpired)) ); + // Exactly at the computed unlock ledger. + env.ledger().with_mut(|li| li.sequence_number = 26_280); + // assert on expected pass/fail behavior here, matching the contract's boundary convention. + env.ledger().with_mut(|li| li.sequence_number = 26_281); client.withdraw_split(&cohost, &event_id);🤖 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/payments/src/revenue_split_test.rs` around lines 752 - 802, Update test_withdraw_split_respects_admin_delay_extension_after_multiple_extensions to assert that withdrawal at ledger 26,280 still returns PaymentError::EscrowNotExpired, then advance to ledger 26,281 and preserve the successful withdrawal assertion.contracts/event/src/integration_tests.rs (2)
1026-1070: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider asserting no side effects on the failure path.
The test correctly validates the error variant at deadline+1, but doesn't confirm balances/payment status remain untouched after the rejected call — cheap to add and rules out partial-state mutation bugs.
♻️ Suggested addition
let res = payments_client.try_request_postponement_refund(&attendee, &t); assert_eq!( res.err(), Some(Ok( payments_contract::PaymentError::PostponementWindowClosed )) ); + assert_eq!(_token_client.balance(&attendee), 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/event/src/integration_tests.rs` around lines 1026 - 1070, Extend test_postponement_refund_one_ledger_past_deadline_fails to capture the attendee’s token balance and payment status before try_request_postponement_refund, then assert both remain unchanged after the PostponementWindowClosed error. Use the existing balance and payment-status accessors available through payments_client or the token client.
1080-1138: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicates
setup_linked's cross-contract wiring instead of reusing it.This block re-implements the same event/ticket/payments registration and initialization already in
setup_linked(lines 326-377), just to add two extra calls. Consider extendingsetup_linked(or adding a small wrapper) to includeticket_client.set_payments_contract/payments_client.set_ticket_contractso resale-oriented tests can reuse it.♻️ Suggested refactor direction
fn setup_linked( env: &Env, ) -> ( /* ... */ ) { // ... existing wiring ... + ticket_client.set_payments_contract(&organizer, &payments_contract_id); + payments_client.set_ticket_contract(&organizer, &ticket_contract_id); (event_client, payments_client, ticket_client, token_client, token_admin_client, token_address, organizer, payments_contract_id) }🤖 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/event/src/integration_tests.rs` around lines 1080 - 1138, Refactor the resale test setup to reuse setup_linked instead of duplicating contract registration and cross-contract initialization. Extend setup_linked, or add a focused wrapper around it, to perform ticket_client.set_payments_contract and payments_client.set_ticket_contract, then update this test to use that shared setup while preserving its event-specific configuration.
🤖 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.
Nitpick comments:
In `@contracts/event/src/integration_tests.rs`:
- Around line 1026-1070: Extend
test_postponement_refund_one_ledger_past_deadline_fails to capture the
attendee’s token balance and payment status before
try_request_postponement_refund, then assert both remain unchanged after the
PostponementWindowClosed error. Use the existing balance and payment-status
accessors available through payments_client or the token client.
- Around line 1080-1138: Refactor the resale test setup to reuse setup_linked
instead of duplicating contract registration and cross-contract initialization.
Extend setup_linked, or add a focused wrapper around it, to perform
ticket_client.set_payments_contract and payments_client.set_ticket_contract,
then update this test to use that shared setup while preserving its
event-specific configuration.
In `@contracts/payments/src/revenue_split_test.rs`:
- Around line 752-802: Update
test_withdraw_split_respects_admin_delay_extension_after_multiple_extensions to
assert that withdrawal at ledger 26,280 still returns
PaymentError::EscrowNotExpired, then advance to ledger 26,281 and preserve the
successful withdrawal assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 41b4ab9e-57b2-4684-953b-6e475c88c447
📒 Files selected for processing (2)
contracts/event/src/integration_tests.rscontracts/payments/src/revenue_split_test.rs
2 participants
Closes #145
Summary
Adds automated test coverage for four high-risk edge cases identified in #145:
choice_deadline_ledgerexactly at the boundary (<=still open) and one ledger past it (closed).sync_event_configon the payments contract has no cap onresale_royalty_bps(unlike the event contract's front door, which caps it at 2000 bps). This test documents current behavior when royalty bps alone exceeds the resale price:seller_proceedsgoes negative, the> 0guard means the seller receives nothing, and the buyer's payment is fully retained by the contract while ownership still transfers.extend_withdrawal_delaycalls are cumulative and correctly gatewithdraw_split.Test plan
cargo test -p payments-contract— 21/21 passedcargo test -p event-contract— 17/17 passedSummary by CodeRabbit