Fix request type amount validation on EVMRequest constructor

[m-Peter]

Closes: #39

[claude]

Code Review: Fix request type amount validation on EVMRequest constructor

Overview

This PR addresses issue #39 by fixing the validation logic in the EVMRequest constructor to properly enforce that CLOSE_YIELDVAULT requests must have amount == 0, while all other request types must have amount > 0.

---

✅ Code Quality & Best Practices

Strengths:

• Clear separation of concerns: Breaking the single compound precondition into two separate if statements with explicit panic calls improves readability and debugging
• Better error messages: Each validation now has its own specific error message, making failures easier to diagnose
• Consistent with Solidity contract: The Solidity closeYieldVault() function passes 0 as the amount (line 880), confirming this validation is correct

Suggestions:

1. Consider using preconditions consistently: While the change from preconditions to runtime if checks works, it creates inconsistency with the existing requestType and status validations (lines 80-86) which still use pre {} blocks. For consistency, consider either:
   • Moving all validations to runtime checks, OR
   • Keeping amount validations in the pre {} block with separate conditions

---

🐛 Potential Bugs & Issues

None identified - The logic is correct:

• Original condition: requestType == CLOSE_YIELDVAULT || amount > 0 allowed CLOSE_YIELDVAULT with any amount (bug)
• New conditions properly enforce:
  • Non-close requests require amount > 0
  • Close requests require amount == 0

---

⚡ Performance Considerations

Impact: Negligible

• Changed from single precondition to two runtime checks
• Since preconditions are also evaluated at runtime in Cadence, the performance difference is minimal
• The validation only runs during struct initialization, not in hot paths

---

🔒 Security Concerns

Enhanced security - This fix prevents:

1. Fund leakage: Users cannot accidentally escrow funds when closing a vault (funds would be lost)
2. Invalid state: Ensures request semantics match the protocol design where CLOSE_YIELDVAULT returns all funds without requiring additional deposits
3. Cross-VM inconsistency: Aligns Cadence validation with Solidity contract behavior

Recommendation: Verify that the Solidity contract also validates this on the EVM side when creating requests.

---

🧪 Test Coverage

Excellent test coverage additions:

1. Invalid request type (testInvalidRequestType):

   • Tests all valid request types with correct amounts ✅
   • Tests invalid request type (99) with expectFailure ✅

2. Zero amount validation (testZeroAmountWithdrawal):

   • Tests CLOSE_YIELDVAULT with zero amount (valid) ✅
   • Tests CLOSE_YIELDVAULT with positive amount (should fail) ✅
   • Tests CREATE/DEPOSIT/WITHDRAW with zero amount (should fail) ✅

Edge cases covered:

• Boundary values (0 and max request types)
• All four request types
• Both valid and invalid amount combinations

Suggestion: Consider adding a test for UInt256.max amounts to ensure no overflow issues with very large values (though likely not a practical concern).

---

📋 Synchronization with Solidity

Verified alignment:

• ✅ Solidity RequestType enum matches Cadence (CREATE=0, DEPOSIT=1, WITHDRAW=2, CLOSE=3)
• ✅ Solidity closeYieldVault() passes amount=0 (line 880 in FlowYieldVaultsRequests.sol)
• ⚠️ Action needed: Verify the Solidity contract validates that CLOSE_YIELDVAULT requests cannot be created with non-zero amounts on the EVM side. The validation should exist in both layers for defense in depth.

---

📝 Documentation

The code changes are self-documenting with clear error messages. The CLAUDE.md already documents request types correctly.

---

✨ Overall Assessment

Recommendation: APPROVE with minor suggestion

This is a high-quality fix that:

• ✅ Correctly addresses the reported issue
• ✅ Includes comprehensive test coverage
• ✅ Improves error messaging
• ✅ Maintains security invariants
• ✅ Aligns with cross-VM contract design

Minor suggestion: Consider adding Solidity-side validation for the same constraint to ensure defense-in-depth, or document if it already exists.

---

Files changed:

• cadence/contracts/FlowYieldVaultsEVM.cdc (contract logic) ✅
• cadence/tests/error_handling_test.cdc (test coverage) ✅

Great work @m-Peter! 🎉

[liobrasil]

LGTM ! Good catch

[claude]

PR Review: Fix request type amount validation on EVMRequest constructor

Summary

This PR addresses issue #39 by improving the validation logic for the EVMRequest constructor to ensure that:

1. CLOSE_YIELDVAULT requests must have an amount of 0 (since closing always returns all funds)
2. All other request types (CREATE, DEPOSIT, WITHDRAW) must have an amount > 0

The previous validation used a compound condition that could allow invalid states. This change splits the validation into two explicit checks with clear error messages.

---

✅ Code Quality & Best Practices

Strengths:

• Clear separation of concerns: The validation logic is now split into two distinct checks, making it easier to understand and maintain
• Improved error messages: Each validation failure now has a specific, descriptive error message
• The change from pre conditions to runtime if/panic is appropriate here since these are business logic validations rather than contract invariants

Minor consideration:

• The validation could alternatively use pre conditions with separate blocks, but the current approach with if/panic is acceptable and provides more flexibility for error messaging

---

✅ Potential Bugs & Issues

No bugs identified. The logic is sound:

// Before (combined condition):
requestType == FlowYieldVaultsEVM.RequestType.CLOSE_YIELDVAULT.rawValue || amount > 0

// After (explicit conditions):
if requestType != FlowYieldVaultsEVM.RequestType.CLOSE_YIELDVAULT.rawValue && amount == 0 {
    panic("Amount must be greater than 0...")
}
if requestType == FlowYieldVaultsEVM.RequestType.CLOSE_YIELDVAULT.rawValue && amount > 0 {
    panic("Amount must be equal to 0...")
}

The new logic correctly enforces:

• CLOSE_YIELDVAULT (type 3): amount MUST be 0
• CREATE/DEPOSIT/WITHDRAW (types 0,1,2): amount MUST be > 0

This aligns with the Solidity side where closeYieldVault() passes 0 as the amount (FlowYieldVaultsRequests.sol:883).

---

✅ Performance Considerations

No performance concerns:

• The validation logic adds minimal overhead (two simple comparisons)
• These checks happen at struct initialization, which is unavoidable
• The change from one compound condition to two separate checks is negligible in terms of performance

---

✅ Security Concerns

Security improved:

• Prevents invalid state: The previous logic could theoretically allow a CLOSE_YIELDVAULT request with a non-zero amount, which would be semantically incorrect
• Clear validation boundaries: Explicit checks make it easier to audit and verify that the contract enforces correct business logic
• Consistent with EVM side: The Solidity contract already enforces amount = 0 for CLOSE_YIELDVAULT (FlowYieldVaultsRequests.sol:883), so this change ensures Cadence-side consistency

No new vulnerabilities introduced.

---

✅ Test Coverage

Excellent test coverage:

The PR adds comprehensive tests in error_handling_test.cdc:

1. ✅ Tests that CLOSE_YIELDVAULT with positive amount fails (lines 117-131)
2. ✅ Tests that CREATE/DEPOSIT/WITHDRAW with zero amount fail (lines 139-155)
3. ✅ Tests that invalid request type (99) fails (lines 78-92)
4. ✅ Updated existing tests to use correct amounts per request type (lines 50-54)

Test quality:

• Uses Test.expectFailure() correctly to verify panics
• Checks error message substrings to ensure the right validation is triggered
• Covers all four request types with appropriate test cases
• Loops through request types to avoid duplication (DRY principle)

---

📋 Recommendations

Optional improvements (not blockers):

1. Consider adding a comment in Solidity contract to document the validation expectations:

   // In FlowYieldVaultsRequests.sol near closeYieldVault()
   // NOTE: CLOSE_YIELDVAULT always uses amount=0 as it returns all vault funds

2. Consider documenting the validation rules in CLAUDE.md under the "Request Types" section to help future contributors understand these constraints.

---

✅ Final Verdict

This PR is ready to merge.

The changes are:

• ✅ Logically correct
• ✅ Well-tested with comprehensive coverage
• ✅ Improve code clarity and maintainability
• ✅ Enhance security by preventing invalid states
• ✅ Consistent with the Solidity contract behavior
• ✅ Address the stated issue (Fix condition on validating request type for EVMRequest constructor #39) completely

Great work on the thorough test coverage and clear implementation! 🎉