feat: implemented Set TTL on refund and dispute persistent storage entries#159
Merged
anonfedora merged 3 commits intoanonfedora:masterfrom Apr 2, 2026
Merged
Conversation
|
@Ugasutun 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! 🚀 |
📝 WalkthroughWalkthroughA new unit test Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 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: 4
🧹 Nitpick comments (3)
contracts/escrow-manager/src/lib.rs (3)
500-504: Consider extending TTL for released escrows.The
refund_escrowfunction extends TTL for refunded entries (line 591-593), butrelease_funds_on_confirmationdoes not extend TTL for released entries. For consistency and to enable automatic pruning of all resolved escrow records, consider adding TTL extension here as well.♻️ Proposed addition after line 501
escrow.status = EscrowStatus::Released; env.storage().persistent().set(&escrow_id, &escrow); + + // Extend TTL for released entry (30 days / 518400 ledgers) + env.storage() + .persistent() + .extend_ttl(&escrow_id, DEFAULT_TTL_LEDGER_COUNT, DEFAULT_TTL_LEDGER_COUNT); env.events() .publish((symbol_short!("esc_rel"),), (escrow_id,));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/escrow-manager/src/lib.rs` around lines 500 - 504, The release_funds_on_confirmation path sets escrow.status = EscrowStatus::Released and persists it but doesn't extend the TTL like refund_escrow does; update the release_funds_on_confirmation logic to call the same TTL-extension helper or storage API after env.storage().persistent().set(&escrow_id, &escrow) (or use the same function used in refund_escrow) so released escrows get the persistent TTL extension and can be pruned consistently.
664-667: Error code reuse may cause confusion.
EscrowNotActiveis returned when the escrow status is notDisputed, but the escrow could actually beActive,Released, orRefunded. Reusing this error makes it harder for callers to distinguish between "escrow is active but not disputed" vs "escrow doesn't exist" vs "escrow was already resolved".Consider adding a specific error variant like
EscrowNotDisputedto theContractErrorenum for clearer error semantics.♻️ Proposed fix
Add to
ContractErrorenum:EscrowNotDisputed = 13,Then update the check:
// Only allow TTL renewal for active disputes if escrow.status != EscrowStatus::Disputed { - return Err(ContractError::EscrowNotActive); + return Err(ContractError::EscrowNotDisputed); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/escrow-manager/src/lib.rs` around lines 664 - 667, Add a new error variant EscrowNotDisputed to the ContractError enum and update the TTL-renewal check to return that new variant instead of EscrowNotActive; specifically, add EscrowNotDisputed to the ContractError definition (matching the enum's encoding style) and change the guard in the function that currently checks escrow.status != EscrowStatus::Disputed to return Err(ContractError::EscrowNotDisputed). Also search for other uses of EscrowNotActive that currently represent the "not disputed" case (and update tests/docs accordingly) to keep semantics clear.
614-683: Missing test coverage for dispute functions.The new
dispute_escrowandrenew_dispute_ttlfunctions lack corresponding test cases. Critical scenarios to cover:
dispute_escrowsuccess with each authorized party (buyer, seller, lender)dispute_escrowrejection for unauthorized callerdispute_escrowrejection when escrow is notActiverenew_dispute_ttlsuccess for active disputerenew_dispute_ttlrejection when escrow is notDisputedrenew_dispute_ttlrejection for unauthorized callerWould you like me to generate the test cases for these scenarios?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/escrow-manager/src/lib.rs` around lines 614 - 683, Add unit tests covering dispute_escrow and renew_dispute_ttl: create test cases that (1) call dispute_escrow successfully as buyer, seller, and lender by setting up an escrow with EscrowStatus::Active and appropriate buyer/seller/lender Addresses, asserting Ok result, status becomes EscrowStatus::Disputed, event (symbol_short!("esc_dsp")) is emitted and TTL was extended; (2) call dispute_escrow with an unauthorized Address and assert it returns ContractError::Unauthorized; (3) call dispute_escrow when escrow.status != EscrowStatus::Active and assert ContractError::EscrowNotActive; (4) call renew_dispute_ttl successfully when escrow.status == EscrowStatus::Disputed from buyer/seller/lender and assert Ok, event (symbol_short!("dsp_rnew")) emitted and TTL extended; (5) call renew_dispute_ttl when escrow.status != EscrowStatus::Disputed and assert ContractError::EscrowNotActive; and (6) call renew_dispute_ttl from an unauthorized Address and assert ContractError::Unauthorized. Use the existing test harness to manipulate Env storage, set Escrow entries by escrow_id, and verify events and TTL via env.storage().persistent() and env.events(); reference functions dispute_escrow and renew_dispute_ttl and enum variants EscrowStatus::Active and EscrowStatus::Disputed when locating code to test.
🤖 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/collateral-registry/src/lib.rs`:
- Around line 711-741: The test currently calls env.mock_all_auths() so
admin.require_auth() always succeeds; replace that with selective mocking and
explicitly invoke verify_collateral as the unauthorized caller: remove
env.mock_all_auths(), call env.mock_auths(...) to only authorize the admin
(e.g., mock_auths with admin allowed and leave unauthorized unmocked), then set
the test caller to the unauthorized address (use env.set_caller or the test
harness method you use to change the caller) before calling
CollateralRegistry::verify_collateral in test_verify_collateral_unauthorized so
the admin.require_auth() check fails and the call returns
Err(ContractError::Unauthorized).
- Line 390: The event symbol passed to symbol_short! is too long
("coll_verified"); update the call in the env.events().publish invocation to use
a symbol of at most 9 characters (e.g., "coll_vrfy") so it satisfies
symbol_short! constraints; locate the publish call that uses symbol_short!
(env.events().publish((symbol_short!("coll_verified"),), (id,));) and replace
the symbol string with a shortened, consistent token such as "coll_vrfy".
In `@contracts/escrow-manager/src/lib.rs`:
- Around line 628-633: Add a dispute-resolution entrypoint that allows an
authorized actor to transition escrows from EscrowStatus::Disputed to a final
state (e.g., EscrowStatus::Cancelled, Released, or Refunded) and perform the
necessary token/account transfers and storage updates; implement a new function
(e.g., resolve_dispute) which validates caller authorization, loads the escrow
by escrow_id, checks escrow.status == EscrowStatus::Disputed, sets escrow.status
to the chosen final status, executes the release/refund logic tied to the Escrow
struct (refund_to, release_to, or similar fields), and persists the updated
escrow with env.storage().persistent().set(&escrow_id, &escrow); ensure you
reuse existing helpers for transfers and permission checks and add tests
asserting transitions from Disputed to the allowed final states and that funds
are moved accordingly.
- Around line 19-24: Update the EscrowStatus enum to align discriminant values
with the rest of the codebase: add missing Pending and Cancelled variants and
set numeric values to Pending=0, Active=1, Released=2, Cancelled=3, Disputed=4
in the EscrowStatus enum; if the previous Refunded variant must be preserved for
backward compatibility, make Refunded an alias or implement a conversion mapping
so Refunded maps to Cancelled (same discriminant) and update any
serialize/deserialize logic that references EscrowStatus accordingly.
---
Nitpick comments:
In `@contracts/escrow-manager/src/lib.rs`:
- Around line 500-504: The release_funds_on_confirmation path sets escrow.status
= EscrowStatus::Released and persists it but doesn't extend the TTL like
refund_escrow does; update the release_funds_on_confirmation logic to call the
same TTL-extension helper or storage API after
env.storage().persistent().set(&escrow_id, &escrow) (or use the same function
used in refund_escrow) so released escrows get the persistent TTL extension and
can be pruned consistently.
- Around line 664-667: Add a new error variant EscrowNotDisputed to the
ContractError enum and update the TTL-renewal check to return that new variant
instead of EscrowNotActive; specifically, add EscrowNotDisputed to the
ContractError definition (matching the enum's encoding style) and change the
guard in the function that currently checks escrow.status !=
EscrowStatus::Disputed to return Err(ContractError::EscrowNotDisputed). Also
search for other uses of EscrowNotActive that currently represent the "not
disputed" case (and update tests/docs accordingly) to keep semantics clear.
- Around line 614-683: Add unit tests covering dispute_escrow and
renew_dispute_ttl: create test cases that (1) call dispute_escrow successfully
as buyer, seller, and lender by setting up an escrow with EscrowStatus::Active
and appropriate buyer/seller/lender Addresses, asserting Ok result, status
becomes EscrowStatus::Disputed, event (symbol_short!("esc_dsp")) is emitted and
TTL was extended; (2) call dispute_escrow with an unauthorized Address and
assert it returns ContractError::Unauthorized; (3) call dispute_escrow when
escrow.status != EscrowStatus::Active and assert ContractError::EscrowNotActive;
(4) call renew_dispute_ttl successfully when escrow.status ==
EscrowStatus::Disputed from buyer/seller/lender and assert Ok, event
(symbol_short!("dsp_rnew")) emitted and TTL extended; (5) call renew_dispute_ttl
when escrow.status != EscrowStatus::Disputed and assert
ContractError::EscrowNotActive; and (6) call renew_dispute_ttl from an
unauthorized Address and assert ContractError::Unauthorized. Use the existing
test harness to manipulate Env storage, set Escrow entries by escrow_id, and
verify events and TTL via env.storage().persistent() and env.events(); reference
functions dispute_escrow and renew_dispute_ttl and enum variants
EscrowStatus::Active and EscrowStatus::Disputed when locating code to test.
🪄 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: 05829196-8387-4294-9b98-1774f2521d38
📒 Files selected for processing (2)
contracts/collateral-registry/src/lib.rscontracts/escrow-manager/src/lib.rs
Owner
|
Fix failing CI and fix merge conflicts, please @Ugasutun |
Contributor
Author
|
@anonfedora merge |
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 (2)
contracts/collateral-registry/src/lib.rs (2)
764-835:⚠️ Potential issue | 🔴 CriticalCritical: Malformed test function with unclosed delimiter.
The
test_verify_collateral_unauthorizedfunction starting at line 765 is never properly closed. At line 771, a new functiontest_propose_adminis defined inside it, creating an invalid nested function. This causes the Rust parse error reported by CI.The test function body from lines 812-831 appears to be remnants of multiple tests merged incorrectly:
- Code referencing
ownervariable (lines 821, 823) that's declared at line 770- Code referencing
new_admin(line 833) that's declared at line 775- Mixed logic from
test_verify_collateral_unauthorizedandtest_propose_admin_unauthorizedThis entire section needs to be restructured with proper function boundaries.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/collateral-registry/src/lib.rs` around lines 764 - 835, The test block is malformed: close the fn test_verify_collateral_unauthorized() body before defining fn test_propose_admin() so you don't nest functions; split the merged logic into three separate tests (test_verify_collateral_unauthorized, test_propose_admin, and test_propose_admin_unauthorized), ensuring each has its own env/mock setup and local variables (declare owner in the verify test, new_admin in the propose tests) and move calls like CollateralRegistry::register_collateral, ::verify_collateral, and ::propose_admin into the appropriate test functions so variables (owner, new_admin, collateral_id) are defined in the same function where they are used and no function bodies overlap.
837-857:⚠️ Potential issue | 🔴 CriticalCritical: Two test functions merged together causing parse error.
The changed line 838 (
fn test_verify_collateral_not_found()) was inserted incorrectly, creating invalid syntax where two test functions are interleaved:#[test] fn test_verify_collateral_not_found() { // <-- starts here #[should_panic(expected = "HostError: Error(Contract, `#8`)")] // <-- attribute for next fn fn test_accept_admin_no_pending() { // <-- another fn startsThe function
test_verify_collateral_not_foundhas no closing brace before the next test's attribute, and the body content (lines 841-857) is shared/mixed between both functions.🐛 Proposed fix to separate the test functions
#[test] fn test_verify_collateral_not_found() { + let env = Env::default(); + env.mock_all_auths(); + let admin = Address::generate(&env); + let contract_id = env.register_contract(None, CollateralRegistry); + + env.as_contract(&contract_id, || { + CollateralRegistry::initialize(env.clone(), admin).unwrap(); + + // Try to verify non-existent collateral + let result = CollateralRegistry::verify_collateral(env.clone(), 999); + assert_eq!(result, Err(ContractError::CollateralNotFound)); + }); + } + + #[test] #[should_panic(expected = "HostError: Error(Contract, `#8`)")] fn test_accept_admin_no_pending() { let env = Env::default(); env.mock_all_auths(); let admin = Address::generate(&env); let contract_id = env.register_contract(None, CollateralRegistry); - - env.as_contract(&contract_id, || { - CollateralRegistry::initialize(env.clone(), admin).unwrap(); - - // Try to verify non-existent collateral - let result = CollateralRegistry::verify_collateral(env.clone(), 999); - assert_eq!(result, Err(ContractError::CollateralNotFound)); - }); let client = CollateralRegistryClient::new(&env, &contract_id); client.initialize(&admin); client.accept_admin(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/collateral-registry/src/lib.rs` around lines 837 - 857, The file accidentally merged two test functions; split the interleaved tests by closing fn test_verify_collateral_not_found() before the next test attribute and put the #[should_panic(...)] attribute immediately above fn test_accept_admin_no_pending(). Ensure test_verify_collateral_not_found contains only the verify_collateral-related setup/call (calling CollateralRegistry::initialize and CollateralRegistry::verify_collateral and asserting Err(ContractError::CollateralNotFound)), and test_accept_admin_no_pending contains the client initialization and client.accept_admin() with the #[should_panic(expected = "HostError: Error(Contract, `#8`)")] attribute; verify function and client names (CollateralRegistry::verify_collateral, CollateralRegistry::initialize, CollateralRegistryClient::new, client.accept_admin) are used to locate and separate the blocks.
🤖 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 `@contracts/collateral-registry/src/lib.rs`:
- Around line 764-835: The test block is malformed: close the fn
test_verify_collateral_unauthorized() body before defining fn
test_propose_admin() so you don't nest functions; split the merged logic into
three separate tests (test_verify_collateral_unauthorized, test_propose_admin,
and test_propose_admin_unauthorized), ensuring each has its own env/mock setup
and local variables (declare owner in the verify test, new_admin in the propose
tests) and move calls like CollateralRegistry::register_collateral,
::verify_collateral, and ::propose_admin into the appropriate test functions so
variables (owner, new_admin, collateral_id) are defined in the same function
where they are used and no function bodies overlap.
- Around line 837-857: The file accidentally merged two test functions; split
the interleaved tests by closing fn test_verify_collateral_not_found() before
the next test attribute and put the #[should_panic(...)] attribute immediately
above fn test_accept_admin_no_pending(). Ensure test_verify_collateral_not_found
contains only the verify_collateral-related setup/call (calling
CollateralRegistry::initialize and CollateralRegistry::verify_collateral and
asserting Err(ContractError::CollateralNotFound)), and
test_accept_admin_no_pending contains the client initialization and
client.accept_admin() with the #[should_panic(expected = "HostError:
Error(Contract, `#8`)")] attribute; verify function and client names
(CollateralRegistry::verify_collateral, CollateralRegistry::initialize,
CollateralRegistryClient::new, client.accept_admin) are used to locate and
separate the blocks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6a8a162f-ece9-45a9-a878-42e8a20f1149
📒 Files selected for processing (1)
contracts/collateral-registry/src/lib.rs
anonfedora
approved these changes
Apr 2, 2026
Owner
anonfedora
left a comment
anonfedora
left a comment
There was a problem hiding this comment.
Please, go to drips and submit a ticket so you'll be rewarded with points
3 tasks
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.
Closes #150
PR Description: Set TTL on refund and dispute persistent storage entries
Overview
This PR adds Time-To-Live (TTL) management for refund and dispute storage entries in the EscrowManager contract to prevent ledger bloat and reduce storage costs.
Changes Made
1. EscrowStatus Enum Enhancement
Disputed = 3status to track disputed escrows2. TTL Configuration
DEFAULT_TTL_LEDGER_COUNTconstant (518400 ledgers ≈ 30 days)3. TTL Extension for Refund
refund_escrowfunction to extend TTL after refund is processed4. New Function: dispute_escrow
5. New Function: renew_dispute_ttl
6. Storage TTL Extension
Definition of Done (DoD) Checklist
Files Modified
contracts/escrow-manager/src/lib.rsBreaking Changes
dispute_escrowfunction to the contract interfacerenew_dispute_ttlfunction to the contract interfaceDisputedstatus to EscrowStatus enumSummary by CodeRabbit