refactor: finish EscrowContract structured errors and events#448
Conversation
|
@Yash-Karakoti 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! 🚀 |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe EscrowContract refactors from panic-based error handling to explicit ChangesEscrowContract Structured Errors and Events
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 👉 Get your free trial and get 200 agent minutes per Slack user (a $50 value). 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
onchain/contracts/escrow_contract/src/test.rs (1)
641-654:⚠️ Potential issue | 🟡 MinorTest no longer exercises
depositdespite its name.
test_deposit_invalid_amountnow callstry_withdrawfor both the0and-50cases (the// or try_depositcomments confirm the substitution). This is a coverage regression: the deposit path'samount <= 0guard is no longer asserted here, and it duplicatestest_withdraw_invalid_amount.test_deposit_zero_panicscovers the zero case but the negative-amount case fordepositis now untested.🔧 Proposed fix: restore `try_deposit` coverage
- let result0 = client.try_withdraw(&from, &0); // or try_deposit - assert_eq!(result0, Err(Ok(EscrowError::InvalidAmount))); - - let result_neg = client.try_withdraw(&from, &-50); // or try_deposit - assert_eq!(result_neg, Err(Ok(EscrowError::InvalidAmount))); + let result0 = client.try_deposit(&from, &0); + assert_eq!(result0, Err(Ok(EscrowError::InvalidAmount))); + + let result_neg = client.try_deposit(&from, &-50); + assert_eq!(result_neg, Err(Ok(EscrowError::InvalidAmount)));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@onchain/contracts/escrow_contract/src/test.rs` around lines 641 - 654, The test named test_deposit_invalid_amount currently calls client.try_withdraw instead of exercising the deposit path; change both occurrences of client.try_withdraw(&from, &0) and client.try_withdraw(&from, &-50) back to client.try_deposit(&from, &0) and client.try_deposit(&from, &-50) respectively so the deposit guard (deposit/try_deposit) is asserted and returns Err(Ok(EscrowError::InvalidAmount)) for both zero and negative amounts; keep the rest of the setup (setup_test, create_vault, EscrowError::InvalidAmount, owner/from variables) unchanged.
🧹 Nitpick comments (4)
onchain/contracts/escrow_contract/src/test.rs (1)
271-272: Misleading_panicssuffixes after the Result-based migration.Several tests still carry
_panicsin their name but now assertErr(Ok(EscrowError::...))rather than panics (e.g.,test_schedule_payment_past_release_panics,test_schedule_payment_insufficient_balance_panics,test_execute_scheduled_early_panics,test_execute_scheduled_double_panics,test_execute_scheduled_not_found_panics,test_deposit_zero_panics). Consider renaming to*_returns_error(or similar) for accuracy. The remaining#[should_panic]tests (auth failures, non-owner) can keep the_panicssuffix.Also applies to: 291-292, 404-405, 529-530, 559-560, 783-784
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@onchain/contracts/escrow_contract/src/test.rs` around lines 271 - 272, Rename the misleading test functions that still use the _panics suffix to reflect they now return Err results; for example rename test_schedule_payment_past_release_panics to test_schedule_payment_past_release_returns_error and similarly update test_schedule_payment_insufficient_balance_panics, test_execute_scheduled_early_panics, test_execute_scheduled_double_panics, test_execute_scheduled_not_found_panics, and test_deposit_zero_panics to *_returns_error (or another consistent *_returns_err naming); update their function identifiers and any references in the test module so the names match the new behavior while leaving true #[should_panic] tests (auth/non-owner) unchanged.onchain/tests/e2e/test_full_flow.rs (1)
131-131: Optional: prefer.expect("...")over.unwrap()in test setup.Using
.expect("create_vault should succeed")(and similar for the recipient vault on line 166) provides a clearer failure message if the test ever regresses, without changing semantics.Also applies to: 166-166
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@onchain/tests/e2e/test_full_flow.rs` at line 131, Replace panic-style unwrap() calls in test setup with expect(...) messages to give clearer failure context: change the EscrowContract::create_vault(...).unwrap() invocation to use .expect("create_vault should succeed") and do the same for the recipient vault creation call later (the other EscrowContract::create_vault(...) invocation) using a descriptive message like .expect("create_recipient_vault should succeed").onchain/contracts/escrow_contract/src/lib.rs (2)
168-169: Inconsistent: raw-=here while other paths usechecked_sub.
deposit,withdraw, andtrigger_auto_paywere updated to usechecked_add/checked_submapped toEscrowError::ArithmeticError, butschedule_paymentstill usesstate.balance -= amount. The guard at line 164 (state.balance < amount) makes this safe today, but usingchecked_subhere would match the PR's stated goal of replacing arithmetic that can panic and would harden against future refactors that loosen the precondition.♻️ Proposed fix
- state.balance -= amount; + state.balance = state + .balance + .checked_sub(amount) + .ok_or(EscrowError::ArithmeticError)?; write_vault_state(&env, &from, &state);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@onchain/contracts/escrow_contract/src/lib.rs` around lines 168 - 169, The subtraction in schedule_payment currently uses raw state.balance -= amount while other paths use checked arithmetic; replace that in schedule_payment by using state.balance.checked_sub(amount).ok_or(EscrowError::ArithmeticError)? and propagate the error (matching deposit/withdraw/trigger_auto_pay behavior), then call write_vault_state(&env, &from, &state) as before so the function consistently uses checked_sub and returns EscrowError::ArithmeticError on underflow.
388-391: Minor: consider a more accurate error forresolveon missing recipient vault.
resolveis called forpayment.toinexecute_scheduledandauto_pay.tointrigger_auto_pay. If the recipient hasn't set up a vault, the caller now seesEscrowError::VaultNotFound, which is ambiguous with the source-vault check earlier in those flows. A dedicated variant (e.g.,RecipientVaultNotFound) — or at least a doc comment clarifying the meaning — would make the failure mode unambiguous for off-chain consumers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@onchain/contracts/escrow_contract/src/lib.rs` around lines 388 - 391, The resolve function currently maps a missing recipient vault to EscrowError::VaultNotFound which is ambiguous with source-vault errors in execute_scheduled and trigger_auto_pay; add a distinct EscrowError::RecipientVaultNotFound (or similarly named variant) to the EscrowError enum and have resolve return that variant when read_vault_config(env, commitment) is None, then update any error propagation/handling and tests that expect VaultNotFound so off-chain callers can unambiguously detect a missing recipient vault; update doc comments for resolve/read_vault_config to note this behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@onchain/contracts/escrow_contract/src/test.rs`:
- Around line 641-654: The test named test_deposit_invalid_amount currently
calls client.try_withdraw instead of exercising the deposit path; change both
occurrences of client.try_withdraw(&from, &0) and client.try_withdraw(&from,
&-50) back to client.try_deposit(&from, &0) and client.try_deposit(&from, &-50)
respectively so the deposit guard (deposit/try_deposit) is asserted and returns
Err(Ok(EscrowError::InvalidAmount)) for both zero and negative amounts; keep the
rest of the setup (setup_test, create_vault, EscrowError::InvalidAmount,
owner/from variables) unchanged.
---
Nitpick comments:
In `@onchain/contracts/escrow_contract/src/lib.rs`:
- Around line 168-169: The subtraction in schedule_payment currently uses raw
state.balance -= amount while other paths use checked arithmetic; replace that
in schedule_payment by using
state.balance.checked_sub(amount).ok_or(EscrowError::ArithmeticError)? and
propagate the error (matching deposit/withdraw/trigger_auto_pay behavior), then
call write_vault_state(&env, &from, &state) as before so the function
consistently uses checked_sub and returns EscrowError::ArithmeticError on
underflow.
- Around line 388-391: The resolve function currently maps a missing recipient
vault to EscrowError::VaultNotFound which is ambiguous with source-vault errors
in execute_scheduled and trigger_auto_pay; add a distinct
EscrowError::RecipientVaultNotFound (or similarly named variant) to the
EscrowError enum and have resolve return that variant when
read_vault_config(env, commitment) is None, then update any error
propagation/handling and tests that expect VaultNotFound so off-chain callers
can unambiguously detect a missing recipient vault; update doc comments for
resolve/read_vault_config to note this behavior.
In `@onchain/contracts/escrow_contract/src/test.rs`:
- Around line 271-272: Rename the misleading test functions that still use the
_panics suffix to reflect they now return Err results; for example rename
test_schedule_payment_past_release_panics to
test_schedule_payment_past_release_returns_error and similarly update
test_schedule_payment_insufficient_balance_panics,
test_execute_scheduled_early_panics, test_execute_scheduled_double_panics,
test_execute_scheduled_not_found_panics, and test_deposit_zero_panics to
*_returns_error (or another consistent *_returns_err naming); update their
function identifiers and any references in the test module so the names match
the new behavior while leaving true #[should_panic] tests (auth/non-owner)
unchanged.
In `@onchain/tests/e2e/test_full_flow.rs`:
- Line 131: Replace panic-style unwrap() calls in test setup with expect(...)
messages to give clearer failure context: change the
EscrowContract::create_vault(...).unwrap() invocation to use
.expect("create_vault should succeed") and do the same for the recipient vault
creation call later (the other EscrowContract::create_vault(...) invocation)
using a descriptive message like .expect("create_recipient_vault should
succeed").
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 98913aab-917d-4d0b-8941-a68f566b7b2f
📒 Files selected for processing (5)
onchain/contracts/escrow_contract/src/events.rsonchain/contracts/escrow_contract/src/lib.rsonchain/contracts/escrow_contract/src/test.rsonchain/shared/src/errors.rsonchain/tests/e2e/test_full_flow.rs
2 participants
🚀 Alien Protocol Pull Request
📌 Type of Change
📝 Changes description
What changed and why:
Refactored the
EscrowContractto use structured errors and events instead of panic-based control flows. This improves contract safety, predictability, and aligns with the project's Result-based API standards.Context & Trade-offs:
panic_with_error!macros with explicitResult<_, EscrowError>returns across all public APIs (initialize, create_vault, deposit, withdraw, trigger_auto_pay, etc.).ArithmeticErrorvariant toEscrowErrorto handle math operations safely viachecked_addandchecked_subinstead of using.expect().vault_crtevent to use the#[contractevent]structured pattern.test.rsto expect standard explicit errors rather than trapped panics.Steps to test or verify the change:
onchain/contracts/escrow_contractdirectory.cargo clippy --all-targets --all-features -- -D warningsto verify no linter warnings.cargo testto verify all success, failure, and error-handling tests pass.📸 Evidence
Evidence video recording for tests
⏰ Time spent breakdown
4 hours (Refactoring panic flows, updating math operations, standardizing events, and rewriting test assertions).
🌌 Comments
All Smart Contract CI checks pass cleanly locally.
Summary by CodeRabbit
Release Notes
Refactor
Tests