MF-L06: fix(contracts): clear stale challengeExpireAt on cooperative escrow finalization

[nksazonov]

Problem

_applyEscrowDepositEffects() and _applyEscrowWithdrawalEffects() only updated meta.challengeExpireAt when effects.newChallengeExpiry > 0. Since cooperative finalization leaves newChallengeExpiry at its zero default, this condition was never true — meaning a prior non-zero challengeExpireAt set during a challenge was silently left in storage after finalization.

The result: a cooperatively finalized escrow (transitioning from DISPUTED to FINALIZED) would still report a non-zero challengeExpiry through getEscrowDepositData() / getEscrowWithdrawalData(), misleading indexers, SDKs, and UIs.

The unilateral timeout paths cleared challengeExpireAt explicitly, so only the cooperative path from DISPUTED was affected.

Fix

Replace the > 0 guard with a != comparison in both apply helpers, mirroring the pattern already used in the channel apply path:

// before
if (effects.newChallengeExpiry > 0) {
    meta.challengeExpireAt = effects.newChallengeExpiry;
}

// after
if (meta.challengeExpireAt != effects.newChallengeExpiry) {
    meta.challengeExpireAt = effects.newChallengeExpiry;
}

Tests

Regression tests added to:

• test/ChannelHub_units/ChannelHub_finalizeEscrowDeposit.t.sol — test_cooperativeFinalize_fromDISPUTED_clearsChallengeExpiry
• test/ChannelHub_units/ChannelHub_finalizeEscrowWithdrawal.t.sol — test_cooperativeFinalize_fromDISPUTED_clearsChallengeExpiry

Each test: initiates an escrow → challenges it (asserting non-zero challengeExpireAt) → cooperatively finalizes before challenge expires → asserts challengeExpireAt == 0 and status FINALIZED.

Summary by CodeRabbit

• Bug Fixes

  • Fixed escrow challenge expiry handling to ensure the stored expiry value is properly updated in all scenarios, including when resetting to zero.

• Tests

  • Added regression tests verifying cooperative escrow finalization correctly clears challenge expiry values after disputes.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 6a52d0ef-1272-423f-80ca-167decf23e7f

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• ✅ Review completed - (🔄 Check again to review again)

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/mf-l06

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (2)

contracts/test/ChannelHub_units/ChannelHub_finalizeEscrowDeposit.t.sol (1)

9-9: 💤 Low value

Optional: Consider removing unused import.

The EscrowDepositEngine import doesn't appear to be referenced in the test code. While this doesn't affect functionality, removing unused imports can improve code clarity.

♻️ Optional cleanup

-import {EscrowDepositEngine} from "../../src/EscrowDepositEngine.sol";

🤖 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/test/ChannelHub_units/ChannelHub_finalizeEscrowDeposit.t.sol` at
line 9, The import of EscrowDepositEngine is unused in the test
(EscrowDepositEngine) — remove the unused import statement from
ChannelHub_finalizeEscrowDeposit.t.sol to clean up the file; locate the line
importing EscrowDepositEngine and delete it so the test compiles and reads
without the unnecessary symbol.

contracts/test/ChannelHub_units/ChannelHub_finalizeEscrowWithdrawal.t.sol (1)

9-9: 💤 Low value

Optional: Consider removing unused import.

The EscrowWithdrawalEngine import doesn't appear to be referenced in the test code. While this doesn't affect functionality, removing unused imports can improve code clarity.

♻️ Optional cleanup

-import {EscrowWithdrawalEngine} from "../../src/EscrowWithdrawalEngine.sol";

🤖 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/test/ChannelHub_units/ChannelHub_finalizeEscrowWithdrawal.t.sol` at
line 9, Remove the unused import of EscrowWithdrawalEngine from the test file by
deleting the import line that references EscrowWithdrawalEngine; the symbol
EscrowWithdrawalEngine is not referenced anywhere in
ChannelHub_finalizeEscrowWithdrawal tests, so removing that unused import cleans
up the file.

🤖 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/test/ChannelHub_units/ChannelHub_finalizeEscrowDeposit.t.sol`:
- Line 9: The import of EscrowDepositEngine is unused in the test
(EscrowDepositEngine) — remove the unused import statement from
ChannelHub_finalizeEscrowDeposit.t.sol to clean up the file; locate the line
importing EscrowDepositEngine and delete it so the test compiles and reads
without the unnecessary symbol.

In `@contracts/test/ChannelHub_units/ChannelHub_finalizeEscrowWithdrawal.t.sol`:
- Line 9: Remove the unused import of EscrowWithdrawalEngine from the test file
by deleting the import line that references EscrowWithdrawalEngine; the symbol
EscrowWithdrawalEngine is not referenced anywhere in
ChannelHub_finalizeEscrowWithdrawal tests, so removing that unused import cleans
up the file.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c0514c21-8991-4afc-8065-94093bc7369c

📥 Commits

Reviewing files that changed from the base of the PR and between 0cbb9f8 and e245365.

📒 Files selected for processing (3)

• contracts/src/ChannelHub.sol
• contracts/test/ChannelHub_units/ChannelHub_finalizeEscrowDeposit.t.sol
• contracts/test/ChannelHub_units/ChannelHub_finalizeEscrowWithdrawal.t.sol