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

[m-Peter]

Closes: #26

[claude]

Code Review for PR #42: Use single mapping for tracking YieldVault user ownership

Summary

This PR successfully consolidates the dual ownership tracking system (yieldVaultsByEVMAddress array + yieldVaultOwnershipLookup map) into a single yieldVaultOwnershipLookup mapping. This is a solid refactoring that reduces storage costs and code complexity.

---

✅ Strengths

1. Storage Optimization

• Eliminates redundant yieldVaultsByEVMAddress array that duplicated information already in yieldVaultOwnershipLookup
• Adds cleanup logic in processCloseYieldVault (FlowYieldVaultsEVM.cdc:851-853) to remove empty dictionaries, further optimizing storage costs
• Single source of truth reduces maintenance burden and potential inconsistencies

2. Consistent O(1) Operations

• Ownership verification (doesEVMAddressOwnYieldVault) remains O(1) via containsKey check (FlowYieldVaultsEVM.cdc:1651)
• CREATE operations maintain O(1) insertion (FlowYieldVaultsEVM.cdc:793)
• CLOSE operations maintain O(1) removal (FlowYieldVaultsEVM.cdc:849)

3. Comprehensive Documentation Updates

• Updated all relevant docs: FLOW_YIELD_VAULTS_EVM_BRIDGE_DESIGN.md, FRONTEND_INTEGRATION.md, TESTING.md
• Fixed function naming in tests (testYieldVaultOwnershipLookupMapping)
• Updated all scripts to reference the correct mapping

4. Backward Compatibility

• Public API (getYieldVaultIdsForEVMAddress, doesEVMAddressOwnYieldVault) signatures unchanged
• External integrations remain unaffected

---

⚠️ Issues & Concerns

1. CRITICAL: Ordering Guarantees Removed
The documentation update at FlowYieldVaultsEVM.cdc:1633 notes:

"Array of YieldVault Ids owned by the address (order is not guaranteed)"

Impact:

• The old yieldVaultsByEVMAddress array maintained insertion order
• The new implementation returns .keys from a dictionary, which has undefined order in Cadence
• If any frontend or external system relies on chronological ordering (e.g., displaying "oldest YieldVault first"), this will break existing behavior

Questions:

• Are there any UIs or scripts that depend on YieldVault ordering?
• Should we add a creation timestamp to preserve sort capability?

2. View Function Optimization
The new getYieldVaultIdsForEVMAddress implementation (FlowYieldVaultsEVM.cdc:1634-1640) is safe but could use minor cleanup:

// Current implementation
access(all) view fun getYieldVaultIdsForEVMAddress(_ evmAddress: String): [UInt64] {
    if !self.yieldVaultOwnershipLookup.containsKey(evmAddress) {
        return []
    }
    return self.yieldVaultOwnershipLookup[evmAddress]!.keys
}

Suggestion: Use optional binding for clarity:

access(all) view fun getYieldVaultIdsForEVMAddress(_ evmAddress: String): [UInt64] {
    return self.yieldVaultOwnershipLookup[evmAddress]?.keys ?? []
}

3. Unused Return Values
Multiple instances of discarding insert and remove return values:

• FlowYieldVaultsEVM.cdc:793: let _ = FlowYieldVaultsEVM.yieldVaultOwnershipLookup[evmAddr]!.insert(key: yieldVaultId, true)
• FlowYieldVaultsEVM.cdc:849: let _ = FlowYieldVaultsEVM.yieldVaultOwnershipLookup[evmAddr]!.remove(key: request.yieldVaultId)

Issue: The return value indicates whether the key already existed. Discarding it means we're not detecting:

• Duplicate YieldVault ID assignments (insert returns previous value if key existed)
• Removal of non-existent YieldVault IDs (remove returns nil if key didn't exist)

Recommendation: Add assertions to catch logic errors:

// On insert - should never overwrite existing
assert(
    FlowYieldVaultsEVM.yieldVaultOwnershipLookup[evmAddr]!.insert(key: yieldVaultId, true) == nil,
    message: "YieldVault ID already registered for this address"
)

// On remove - should always exist
assert(
    FlowYieldVaultsEVM.yieldVaultOwnershipLookup[evmAddr]!.remove(key: request.yieldVaultId) != nil,
    message: "YieldVault ID not found in ownership lookup"
)

4. Empty Dictionary Cleanup Edge Case
FlowYieldVaultsEVM.cdc:851-853:

if FlowYieldVaultsEVM.yieldVaultOwnershipLookup[evmAddr]!.length == 0 {
    let _ = FlowYieldVaultsEVM.yieldVaultOwnershipLookup.remove(key: evmAddr)
}

This cleanup only happens on CLOSE operations. If the sequence is:

1. CREATE YieldVault (creates entry)
2. CLOSE YieldVault (removes entry + dictionary)
3. CREATE YieldVault again (recreates dictionary)

Potential issue: The check at line 790 (if FlowYieldVaultsEVM.yieldVaultOwnershipLookup[evmAddr] == nil) will fail because we just removed the dictionary. However, this is handled correctly by the nil check, so no bug exists - just noting the pattern.

---

🔒 Security Assessment

No security vulnerabilities introduced:

• Ownership verification logic unchanged (still O(1) dictionary lookup)
• EVM-side userOwnsYieldVault mapping remains independent and authoritative (FlowYieldVaultsRequests.sol:159)
• Two-phase commit pattern (startProcessing/completeProcessing) unaffected
• Access control unchanged

Parallel state management validated:

• Cadence side: yieldVaultOwnershipLookup (this PR)
• EVM side: userOwnsYieldVault (unchanged)
• As documented in CLAUDE.md, both mappings provide O(1) lookups for their respective VMs

---

🧪 Testing Considerations

Test coverage looks good:

• testYieldVaultOwnershipLookupMapping renamed correctly (access_control_test.cdc:121)
• Scripts updated to use new mapping (check_yieldvault_details.cdc, check_yieldvaultmanager_status.cdc, get_contract_state.cdc)

Missing test scenarios:

1. Dictionary cleanup verification: Test that empty dictionaries are removed after closing all YieldVaults
2. Multiple YieldVaults per user: Verify .keys returns all IDs correctly
3. Order independence: Confirm frontends don't break with unordered results

Recommendation: Add integration test:

// Create 3 YieldVaults, close middle one, verify count = 2
// Close remaining 2, verify evmAddress removed from parent map

---

📊 Performance Impact

Positive changes:

• Storage cost reduction: Eliminates array storage (saves ~24 bytes per YieldVault ID)
• Write operations: Removed array append (was O(1) amortized, now pure O(1))
• Read operations: getYieldVaultIdsForEVMAddress now does dictionary .keys extraction instead of array copy (both O(n), similar performance)

No regressions expected for typical workloads (<100 YieldVaults per user).

---

✅ Recommendations

Must address before merge:

1. Clarify ordering requirements - Confirm no external dependencies on chronological order, or add creation timestamps

Should address:
2. Add assertions on insert/remove return values to catch logic errors
3. Add test for dictionary cleanup behavior
4. Consider optional binding syntax for getYieldVaultIdsForEVMAddress

Nice to have:
5. Add inline comment explaining why dictionary cleanup is safe (prevents future confusion)

---

📝 Conclusion

This is a well-executed refactoring that achieves its goal of reducing storage overhead and code duplication. The implementation is correct and maintains all functional guarantees.

Approval recommendation: ✅ Approve with minor suggestions

The only potential blocker is the ordering change - please confirm no external systems depend on insertion order before merging.

Great work on maintaining comprehensive documentation updates across all affected files!

---

Review generated with Claude Code