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

[m-Peter]

Closes: #26

[claude]

Pull Request Review

Summary

This PR removes the redundant yieldVaultsByEVMAddress mapping and refactors getYieldVaultIdsForEVMAddress() to use .keys from yieldVaultOwnershipLookup. This eliminates the O(n) removal issue identified in #26 and simplifies the codebase.

---

✅ Strengths

1. Eliminates Performance Issue: Removes the O(n) firstIndex(of:) operation during vault closure (cadence/contracts/FlowYieldVaultsEVM.cdc:843)
2. Reduces State Redundancy: Single source of truth (yieldVaultOwnershipLookup) instead of maintaining two parallel mappings
3. Cleaner Code: 19 deletions vs 10 additions - net reduction in complexity
4. Preserves API: Public view functions maintain identical signatures

---

🔍 Code Quality Review

Implementation Changes (cadence/contracts/FlowYieldVaultsEVM.cdc:1622-1642)

Before:

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

After:

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

Analysis:

• ✅ Correctly handles missing address case
• ✅ Dictionary .keys returns array of UInt64
• ⚠️ Ordering consideration: Dictionary .keys may return elements in different order than the original array implementation. This could affect any code that depends on insertion order.

Ownership Check Refactor (cadence/contracts/FlowYieldVaultsEVM.cdc:1636-1642)

Before:

access(all) view fun doesEVMAddressOwnYieldVault(evmAddress: String, yieldVaultId: UInt64): Bool {
    if let ownershipMap = self.yieldVaultOwnershipLookup[evmAddress] {
        return ownershipMap[yieldVaultId] ?? false
    }
    return false
}

After:

access(all) view fun doesEVMAddressOwnYieldVault(evmAddress: String, yieldVaultId: UInt64): Bool {
    if \!self.yieldVaultOwnershipLookup.containsKey(evmAddress) {
        return false
    }
    return self.yieldVaultOwnershipLookup[evmAddress]\!.containsKey(yieldVaultId)
}

Analysis:

• ✅ More explicit and readable
• ✅ Uses containsKey which is semantically clearer than ?? false
• ✅ Maintains O(1) performance

---

🔒 Security & Correctness

✅ Cross-VM Synchronization Intact

The Solidity side (FlowYieldVaultsRequests.sol) maintains its own parallel tracking with yieldVaultsByUser and userOwnsYieldVault mappings (solidity/src/FlowYieldVaultsRequests.sol:156-159). This is correct because:

• EVM contract needs local state for validations (_validateYieldVaultOwnership)
• Cadence worker is authoritative for actual vault operations
• Registration happens via _registerYieldVault during successful completeProcessing (solidity/src/FlowYieldVaultsRequests.sol:1150-1156)
• Unregistration happens via _unregisterYieldVault during successful close (solidity/src/FlowYieldVaultsRequests.sol:1159-1160)

⚠️ Potential Ordering Concern

Issue: getYieldVaultIdsForEVMAddress() now returns vault IDs in dictionary order (non-deterministic) instead of insertion order.

Impact Assessment:

• cadence/tests/evm_bridge_lifecycle_test.cdc:61 only checks .length, not order ✅
• cadence/tests/access_control_test.cdc:124 usage needs review
• External consumers (frontend, indexers) may display vaults in different order

Recommendation: Document this behavior change in the function's docstring:

/// @return Array of YieldVault Ids owned by the address (order not guaranteed)

---

🧪 Test Coverage

Existing Tests

• ✅ cadence/tests/evm_bridge_lifecycle_test.cdc:61 - Tests empty array return
• ✅ cadence/tests/access_control_test.cdc:124 - Tests ownership queries

Missing Test Coverage

Consider adding a test for the specific improvement this PR addresses:

// Test that vault closure doesn't degrade with many vaults
access(all) fun testCloseYieldVaultPerformanceWithManyVaults() {
    // Create 10+ vaults for one user
    // Close one vault
    // Verify operation completes without issues
}

---

📊 Performance Analysis

Operation	Before	After	Improvement
Create vault	O(1) array append + O(1) dict insert	O(1) dict insert only	✅ Faster
Close vault	O(n) firstIndex + O(n) remove + O(1) dict remove	O(1) dict remove only	✅ Much faster for users with many vaults
Get vault IDs	O(1) array read	O(n) dict key extraction	⚠️ Slightly slower, but negligible
Check ownership	O(1) dict lookup	O(1) dict lookup	➡️ Same

The tradeoff (slower getYieldVaultIdsForEVMAddress for view queries) is acceptable because:

1. View queries don't cost gas/computation credits
2. Close operations (cadence/contracts/FlowYieldVaultsEVM.cdc:840) happen frequently and did cost real resources
3. Most users won't have enough vaults for O(n) key extraction to be noticeable

---

✅ Best Practices Adherence

1. ✅ Follows "single source of truth" principle
2. ✅ Maintains backward-compatible public API
3. ✅ Removes dead code (cadence/contracts/FlowYieldVaultsEVM.cdc:160, 785-791, 843-845)
4. ✅ Updates inline documentation (cadence/contracts/FlowYieldVaultsEVM.cdc:723)

---

📝 Recommendations

1. Document Ordering Change (Minor)

Update the docstring for getYieldVaultIdsForEVMAddress:

/// @notice Gets all YieldVault Ids owned by an EVM address
/// @param evmAddress The EVM address string to query
/// @return Array of YieldVault Ids owned by the address (order not guaranteed)

2. Consider Migration Note (Optional)

If this is deployed to an existing network with state, document that existing yieldVaultsByEVMAddress data will be abandoned (it's already redundant with yieldVaultOwnershipLookup, so no data loss).

3. Run Full Test Suite

Before merging, ensure:

./local/run_cadence_tests.sh
cd solidity && forge test

---

🎯 Final Assessment

Status: ✅ APPROVE with minor suggestions

This is a well-executed optimization that addresses the performance issue identified in #26. The code is cleaner, more maintainable, and eliminates the O(n) removal issue for power users. The only consideration is documenting the ordering change, which is a non-breaking API modification.

Risk Level: Low

• No breaking changes to external APIs
• No security implications
• Maintains cross-VM synchronization correctness
• Well-isolated change affecting only Cadence contract state management

Great work! 🚀

[claude]

Code Review

This PR addresses issue #26 by removing the redundant yieldVaultsByEVMAddress mapping and using only yieldVaultOwnershipLookup for tracking YieldVault ownership. This is a solid improvement that resolves the O(n) removal cost issue.

✅ Strengths

1. Performance Improvement: Eliminates O(n) firstIndex(of:) operation during vault closure, replacing it with O(1) dictionary removal
2. Reduced Storage: Removes duplicate data structure - the same information was being stored in two places
3. Comprehensive Update: All references across contracts, scripts, and documentation have been properly updated
4. Maintains Functionality: getYieldVaultIdsForEVMAddress() now returns .keys from the lookup map, preserving the same interface

🔍 Code Quality

FlowYieldVaultsEVM.cdc:1624-1630 - The refactored getYieldVaultIdsForEVMAddress():

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

✅ Clean implementation with proper nil-checking

FlowYieldVaultsEVM.cdc:1636-1642 - The refactored doesEVMAddressOwnYieldVault():

access(all) view fun doesEVMAddressOwnYieldVault(evmAddress: String, yieldVaultId: UInt64): Bool {
    if !self.yieldVaultOwnershipLookup.containsKey(evmAddress) {
        return false
    }
    return self.yieldVaultOwnershipLookup[evmAddress]!.containsKey(yieldVaultId)
}

✅ Improved from using ?? false pattern to explicit containsKey() - more idiomatic and efficient

FlowYieldVaultsEVM.cdc:843 - Vault closure cleanup:

FlowYieldVaultsEVM.yieldVaultOwnershipLookup[evmAddr]!.remove(key: request.yieldVaultId)

✅ Now O(1) instead of O(n) - directly addresses the security finding

⚠️ Potential Concerns

1. Ordering Changes: The .keys property of a dictionary in Cadence doesn't guarantee order. Previously, yieldVaultsByEVMAddress maintained insertion order (arrays preserve order). This could affect:

   • Frontend pagination if it relies on consistent ordering
   • Any code that expects YieldVault IDs in creation order

   Impact: Low - Most use cases just need the set of IDs, not a specific order. However, consider documenting this behavior change.

2. Script Output Format: get_contract_state.cdc line 13 now returns the nested map structure instead of simple arrays:

   result["yieldVaultsByEVMAddress"] = FlowYieldVaultsEVM.yieldVaultOwnershipLookup

   Impact: The key name yieldVaultsByEVMAddress is misleading - it now contains {String: {UInt64: Bool}} instead of {String: [UInt64]}. Consider renaming to yieldVaultOwnershipLookup for consistency.

3. Empty Map Cleanup: When the last YieldVault is removed, the empty inner dictionary remains in yieldVaultOwnershipLookup[evmAddr]:

   FlowYieldVaultsEVM.yieldVaultOwnershipLookup[evmAddr]!.remove(key: request.yieldVaultId)
   // If this was the last vault, evmAddr now maps to {}

   Impact: Minimal - just a small storage overhead for users who close all vaults

🧪 Testing

• ✅ Test function renamed appropriately: testYieldVaultsByEVMAddressMapping → testYieldVaultOwnershipLookupMapping
• ⚠️ Consider adding a specific test case to verify:
  • YieldVault IDs are correctly retrievable after creation
  • Removal works correctly (the main fix)
  • Edge case: closing the only vault for an address

📚 Documentation

• ✅ All documentation files updated (FLOW_YIELD_VAULTS_EVM_BRIDGE_DESIGN.md, FRONTEND_INTEGRATION.md)
• ✅ Inline comments updated in processCreateYieldVault (line 723)

🔐 Security

• ✅ Directly addresses FLOW-11 Low severity finding from QuantStamp audit
• ✅ No new attack vectors introduced
• ✅ Maintains access control (same public interfaces)
• ✅ No breaking changes to security model

Recommendations

1. Consider cleaning up empty maps: When removing the last vault, also remove the user's entry:

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

2. Fix script naming: In get_contract_state.cdc, rename the result key from yieldVaultsByEVMAddress to yieldVaultOwnershipLookup for consistency

3. Document ordering change: Add a note to getYieldVaultIdsForEVMAddress() that the array order is not guaranteed

Summary

This is a well-executed refactoring that improves performance and reduces complexity. The changes are thorough and consistent across the codebase. The main trade-off is losing insertion-order guarantees, which should be acceptable for this use case.

Recommendation: Approve with minor suggestions ✅

The core change is sound and ready to merge. The suggestions above are optional improvements that can be addressed in follow-up PRs if desired.

[claude]

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

Overview

This PR successfully removes the redundant yieldVaultsByEVMAddress mapping and consolidates ownership tracking into the single yieldVaultOwnershipLookup mapping. This is a solid refactoring that reduces state redundancy and simplifies maintenance.

✅ Strengths

1. Eliminates Data Redundancy: Removing the duplicate yieldVaultsByEVMAddress array reduces storage costs and eliminates potential sync issues between two data structures tracking the same information.

2. Maintains O(1) Lookup Performance: The ownership verification via doesEVMAddressOwnYieldVault() remains O(1), which is critical for request validation.

3. Comprehensive Updates: All references updated consistently across:

   • Contract implementation (FlowYieldVaultsEVM.cdc:787, 843)
   • Scripts (get_contract_state.cdc, check_yieldvault_details.cdc, check_yieldvaultmanager_status.cdc)
   • Documentation (FLOW_YIELD_VAULTS_EVM_BRIDGE_DESIGN.md, FRONTEND_INTEGRATION.md, TESTING.md)
   • Tests (access_control_test.cdc)

4. Improved Implementation: The refactored getYieldVaultIdsForEVMAddress() (FlowYieldVaultsEVM.cdc:1624-1630) and doesEVMAddressOwnYieldVault() (FlowYieldVaultsEVM.cdc:1636-1642) use cleaner early-return patterns with containsKey() checks.

5. Proper Documentation: The function comment correctly notes "order is not guaranteed" for getYieldVaultIdsForEVMAddress(), which is important since dictionary keys don't maintain insertion order.

⚠️ Considerations

1. Behavioral Change - Unordered Results: The return value of getYieldVaultIdsForEVMAddress() changes from ordered array to unordered dictionary keys. This is correctly documented, but frontend consumers should be aware:

   • Old: yieldVaultsByEVMAddress[addr] returned YieldVault IDs in creation order
   • New: yieldVaultOwnershipLookup[addr]\!.keys returns IDs in arbitrary order

   Impact: If frontends display YieldVaults in creation order, they'll need to sort by ID or fetch timestamps separately. However, this is unlikely to be a breaking issue since YieldVault IDs are monotonically increasing.

2. Script Type Issue: In get_contract_state.cdc:22-23, the code accesses yieldVaultOwnershipLookup[evmAddress]\! which is now a {UInt64: Bool} dictionary, not an array:

   let yieldVaultIds = FlowYieldVaultsEVM.yieldVaultOwnershipLookup[evmAddress]\!
   totalYieldVaults = totalYieldVaults + yieldVaultIds.length

   Issue: This attempts to use .length on a dictionary. In Cadence, dictionaries use .length for the number of keys, so this should still work, but it would be clearer to use .keys.length to match the pattern elsewhere.

3. Potential Edge Case in Close: In processCloseYieldVault() at FlowYieldVaultsEVM.cdc:843, the code removes the key without checking if the map exists or becomes empty:

   FlowYieldVaultsEVM.yieldVaultOwnershipLookup[evmAddr]\!.remove(key: request.yieldVaultId)

   After removing the last YieldVault for an address, the empty dictionary {} remains in storage. Consider cleaning up empty dictionaries to save storage costs:

   FlowYieldVaultsEVM.yieldVaultOwnershipLookup[evmAddr]\!.remove(key: request.yieldVaultId)
   if FlowYieldVaultsEVM.yieldVaultOwnershipLookup[evmAddr]\!.length == 0 {
       FlowYieldVaultsEVM.yieldVaultOwnershipLookup.remove(key: evmAddr)
   }

4. Test Coverage: The test testYieldVaultOwnershipLookupMapping() only checks the empty case. Consider adding a test that:

   • Creates a YieldVault (verifies ID appears in lookup)
   • Verifies doesEVMAddressOwnYieldVault() returns true
   • Closes the YieldVault (verifies ID removed from lookup)

🔒 Security Assessment

No security concerns identified. The refactoring:

• Maintains the same access control patterns
• Preserves ownership verification logic
• Doesn't introduce new attack surfaces
• Keeps the two-phase commit pattern intact

📊 Performance Impact

Neutral to slightly positive:

• Storage: Reduces contract state by eliminating array storage overhead
• Gas costs:
  • getYieldVaultIdsForEVMAddress(): Converts from O(1) array access to O(n) dictionary key extraction, but this is a read-only script function (no gas impact)
  • Create/Close operations: Same complexity (still need to update one mapping)

🎯 Recommendation

Approve with minor suggestions. This is a clean refactoring that achieves its goal. The behavioral change (unordered results) is acceptable and properly documented.

Optional improvements (not blocking):

1. Clean up empty dictionaries in processCloseYieldVault() to reduce storage bloat
2. Clarify .keys.length usage in get_contract_state.cdc for consistency
3. Add integration test covering full create→verify→close lifecycle for ownership tracking

---

Review completed by Claude Code | Closes #26

[claude]

PR Review: Use single mapping for tracking YieldVault user ownership

Summary

This PR removes the redundant yieldVaultsByEVMAddress mapping and consolidates ownership tracking to use only yieldVaultOwnershipLookup. This is a clean refactor that reduces storage costs and simplifies state management while maintaining O(1) ownership lookups.

---

Code Quality ✅

Strengths:

• Clean refactoring: Successfully removes duplicate state while preserving functionality
• Consistent updates: All references (contract, scripts, tests, documentation) are updated systematically
• Clear documentation: Inline comments and doc strings accurately reflect the changes
• Storage optimization: Added cleanup logic in processCloseYieldVault (lines 845-847) to remove empty dictionaries

Observations:

• Line 787: The let _ = pattern is used correctly to discard the optional return value from insert()
• Lines 1629-1633: getYieldVaultIdsForEVMAddress() now returns dictionary keys - important note that order is not guaranteed (correctly documented in line 1627)

---

Potential Issues ⚠️

1. Breaking Change for Frontends (Low severity)
The function getYieldVaultIdsForEVMAddress() now returns keys from a dictionary instead of an array:

• Before: Array with insertion-order preservation
• After: Dictionary keys with no guaranteed order

Impact: Frontends that rely on consistent ordering (e.g., "show newest YieldVault first") may see unexpected changes. However, since the documentation already states "order is not guaranteed" (line 1627), this is acceptable.

Recommendation: Consider adding a release note if any known integrations depend on ordering.

---

2. Dictionary Cleanup Edge Case (Very low severity)
In processCloseYieldVault (lines 845-847), the cleanup logic is well-implemented:

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

However, there's a potential (extremely unlikely) issue: if multiple YieldVaults are closed in the same transaction, and the check happens concurrently, the force-unwrap !.length could panic if another operation removed the key.

Likelihood: Extremely low - would require concurrent modification in the same transaction, which doesn't happen in the current architecture (single-threaded request processing).

Recommendation: No action needed for current implementation, but consider using optional binding if future refactors allow concurrent processing:

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

---

Performance Considerations ✅

Storage Cost Improvements:

• Eliminates redundant array storage (yieldVaultsByEVMAddress)
• Adds cleanup logic to remove empty dictionaries (lines 845-847)
• Reduces overall contract state size

Runtime Performance:

• Maintains O(1) ownership checks via doesEVMAddressOwnYieldVault() (lines 1640-1646)
• getYieldVaultIdsForEVMAddress() remains O(n) where n = number of YieldVaults per user
• Slightly more efficient in processCreateYieldVault (removed array append operation)

Trade-off: Dictionary key extraction in getYieldVaultIdsForEVMAddress() may have slightly different performance characteristics than array access, but difference is negligible for typical user workloads (< 100 YieldVaults per user).

---

Security Concerns ✅

No new security issues identified:

• Ownership validation logic remains unchanged (still using doesEVMAddressOwnYieldVault())
• Force-unwrap safety: All force-unwraps (!) are preceded by existence checks (containsKey, nil checks)
• State consistency: Single source of truth reduces risk of mapping inconsistencies

Positive security change:

• Removed potential for desynchronization between yieldVaultsByEVMAddress and yieldVaultOwnershipLookup

---

Test Coverage ✅

Tests properly updated:

• access_control_test.cdc: Renamed testYieldVaultsByEVMAddressMapping → testYieldVaultOwnershipLookupMapping (lines 121-128)
• Test still validates empty address returns empty array
• Lifecycle test (evm_bridge_lifecycle_test.cdc) continues to use getYieldVaultIdsForEVMAddress()

Recommendations for additional test coverage:

1. Add test for dictionary cleanup on close (verify empty dict is removed)
2. Add test verifying ownership lookup after create/close operations
3. Consider adding test for ordering behavior (document that order is not stable)

---

Documentation Updates ✅

Thoroughly updated:

• ✅ CLAUDE.md: Architecture section updated
• ✅ FLOW_YIELD_VAULTS_EVM_BRIDGE_DESIGN.md: State diagram corrected
• ✅ FRONTEND_INTEGRATION.md: Script example updated
• ✅ TESTING.md: Test name updated
• ✅ All scripts updated to use new mapping

---

Final Recommendation: APPROVE ✅

This is a high-quality refactor that:

• Reduces storage costs and complexity
• Maintains all existing functionality
• Updates documentation comprehensively
• Has no critical security or correctness issues

Minor suggestions (non-blocking):

1. Consider adding release note about ordering change in getYieldVaultIdsForEVMAddress()
2. Add test case verifying dictionary cleanup on close
3. Consider optional binding for cleanup logic (future-proofing)

Great work simplifying the contract state! 🎉

[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