FLOW-1: Prevent double-refund on failed CREATE/DEPOSIT

[liobrasil]

Fixes #16

FLOW-1 is addressed by using a single-path, pull-based refund model (crediting claimableRefunds and requiring the user to claimRefund) rather than mixing direct refunds with escrow credits.

This PR adds regression coverage for the failed DEPOSIT_TO_YIELDVAULT path (CREATE already has coverage) to ensure failures always result in a claimable refund and never re-introduce double-refund semantics.

[claude]

Pull Request Review: FLOW-1 Double-Refund Prevention

Summary

This PR adds regression test coverage for failed DEPOSIT_TO_YIELDVAULT operations to ensure the refund mechanism correctly credits claimableRefunds rather than allowing double-refunds. The test mirrors the existing test_CompleteProcessing_FailureRefundsBalance coverage for CREATE operations.

Code Quality: Excellent

Strengths

1. Clear test naming - The test name explicitly indicates it covers the DEPOSIT_TO_YIELDVAULT path
2. Proper structure - Follows the AAA pattern consistently
3. Comprehensive assertions - Verifies both balance states along with request metadata
4. Code duplication is acceptable - The duplication is justified as it provides explicit coverage for a distinct request type

Security: Strong

Regression Coverage

• Addresses FLOW-1: The test explicitly validates that the single-path refund model is preserved
• No double-refund vectors: By asserting getUserPendingBalance == 0, the test ensures funds cannot be claimed via both direct bridge AND refund claim
• COA fund return validation: The test requires COA to return exactly 1 ether, matching the contract strict validation

Contract Alignment

The test correctly exercises the two-phase commit pattern:

1. startProcessing: Moves funds from pendingUserBalances to COA
2. completeProcessing on failure: COA returns funds, which are credited to claimableRefunds

This matches the Cadence worker behavior.

Test Coverage: Improved

What This Adds

• DEPOSIT_TO_YIELDVAULT failure path: Previously only CREATE had explicit failure refund coverage
• Symmetric testing: Now both deposit-requiring operations have parallel test coverage

Existing Coverage Analysis

• CREATE failure: Line 313
• DEPOSIT failure: Line 335 (this PR)
• CLOSE success: Line 359
• Missing: WITHDRAW failure path, ERC20 token failure paths

Recommendations for Future Coverage

While not blocking for this PR, consider adding:

1. ERC20 failure tests: Both CREATE and DEPOSIT with ERC20 tokens
2. WITHDRAW failure test: Validate that WITHDRAW failures dont create spurious refunds
3. Cadence integration test: Add a test that validates DEPOSIT failure end-to-end

Performance: No Concerns

• Test is lightweight
• No loops or complex computations
• Foundry test execution is fast

Best Practices: Follows Conventions

1. Naming: Follows existing pattern
2. Location: Correctly placed adjacent to related test at line 335
3. Magic numbers: Uses existing constants
4. Prank usage: Properly scoped

Minor Observations

1. Test data alignment: Uses yieldVaultId 42 consistently with other tests
2. Error message: Uses generic Cadence error - acceptable for unit tests
3. No ERC20 variant: This test only covers native FLOW (maintains parity with CREATE test)

Verdict: APPROVE

This PR successfully addresses issue 16 by adding necessary regression coverage for failed DEPOSIT operations. The test is well-structured, security-focused, and maintains consistency with existing test patterns.

Checklist

• Addresses stated issue (FLOW-1 regression coverage)
• No bugs or logic errors
• Follows repository conventions
• Security-conscious assertions
• No performance concerns
• Appropriate test coverage expansion

Recommendation: Merge after CI passes. Consider filing a follow-up issue for the ERC20/WITHDRAW test coverage gaps noted above.