[FLOW-11] Use single mapping for tracking YieldVault user ownership

[m-Peter]

Closes: #26

[claude]

Code Review Summary

This PR successfully consolidates the YieldVault ownership tracking from two separate mappings (yieldVaultsByEVMAddress and yieldVaultOwnershipLookup) into a single yieldVaultRegistry mapping. This is a clean refactoring that reduces redundancy and improves maintainability.

✅ Strengths

1. Simplified Data Structure

• Before: Two mappings were maintained in parallel:
  • yieldVaultsByEVMAddress: {String: [UInt64]} - array of owned vault IDs
  • yieldVaultOwnershipLookup: {String: {UInt64: Bool}} - O(1) ownership checks
• After: Single mapping yieldVaultRegistry: {String: {UInt64: Bool}} serves both purposes
• The getYieldVaultIdsForEVMAddress() now derives the array from dictionary keys, eliminating synchronization concerns

2. Consistent Implementation

All four request types handle the registry correctly:

• CREATE: Initializes nested dictionary if needed, inserts vault ID at FlowYieldVaultsEVM.cdc:791-794
• DEPOSIT: Correctly checks ownership using containsKey() at FlowYieldVaultsEVM.cdc:917-918
• WITHDRAW: Validates ownership before allowing withdrawal at FlowYieldVaultsEVM.cdc:949-950
• CLOSE: Removes vault ID and cleans up empty dictionaries at FlowYieldVaultsEVM.cdc:850-854

3. Storage Optimization

The close operation now includes cleanup logic that removes empty dictionaries, preventing storage bloat from addresses that have closed all their vaults.

4. Improved Ownership Checks

The new pattern using containsKey() is more idiomatic than the previous ownershipMap[id] ?? false

5. Comprehensive Documentation Updates

All relevant documentation files were updated consistently.

🔍 Observations & Minor Issues

1. Return Value Behavior Change

The getYieldVaultIdsForEVMAddress() function now returns unordered results (dictionary keys), whereas the previous array implementation maintained insertion order. The updated documentation correctly notes "order is not guaranteed" at FlowYieldVaultsEVM.cdc:1634.

Impact: If any frontend code relies on the order of vault IDs, this could be a breaking change. However, there's no evidence in the codebase that order matters.

Recommendation: ✅ Acceptable - the documentation update adequately addresses this.

2. Test Coverage

The test testYieldVaultRegistryMapping() only verifies the empty case. It would be beneficial to have integration tests that verify: creating a vault adds to the registry, closing a vault removes from the registry, multiple vaults for the same address, and registry cleanup when all vaults are closed.

Impact: The existing lifecycle tests do exercise these paths indirectly, but explicit registry verification would improve confidence.

Recommendation: 🟡 Consider adding - not blocking, but would strengthen test coverage.

🔒 Security Considerations

✅ No Security Regressions

• Ownership validation logic remains functionally equivalent
• All four request types maintain proper authorization checks
• No new attack vectors introduced
• Storage cleanup in close operation actually reduces potential DoS via storage exhaustion

✅ Parallel EVM Mapping Unchanged

The Solidity side still maintains userOwnsYieldVault[address][yieldVaultId], which provides defense-in-depth.

📊 Performance Considerations

✅ Maintains O(1) Ownership Checks

The core performance characteristic is preserved - doesEVMAddressOwnYieldVault() remains O(1) via dictionary lookup.

🟡 Slight Overhead for List Queries

getYieldVaultIdsForEVMAddress() now extracts keys from a dictionary (O(n) where n = number of vaults for that address) instead of returning a pre-built array (O(1)).

Impact: Negligible for realistic scenarios (users typically have <10 vaults). The memory savings from eliminating the duplicate array far outweigh this minor computational cost.

🎯 Best Practices

✅ Follows Cadence Idioms

• Uses dictionary pattern for sparse mappings
• Implements proper nil-checking with optional binding
• Cleans up empty dictionaries to optimize storage

✅ Maintains Backward Compatibility

All public functions maintain the same signature and semantics (except for order guarantee, which is well-documented).

Verdict: ✅ Approve with Minor Suggestions

This is a well-executed refactoring that:

• Reduces code complexity
• Eliminates synchronization concerns
• Maintains equivalent functionality
• Improves storage efficiency
• Updates all documentation consistently

The implementation is correct, secure, and follows Cadence best practices. The minor observations noted above are not blocking issues.

Optional Follow-ups (Non-blocking)

1. Add explicit registry verification to integration tests
2. Verify frontend integrations handle unordered vault ID arrays (likely already the case)

---

Reviewed by: Claude Code
Review Date: 2026-02-12

[liobrasil]

LGTM !