fix(FLOW-15): Return per-token balances in getPendingRequestsByUserUnpacked

[liobrasil]

Summary

Fixes #30

Previously, getPendingRequestsByUserUnpacked only returned native FLOW balances, which made it harder for clients to surface per-token pending/refund balances.

Note: balanceTokens/pendingBalances/claimableRefundsArray currently cover NATIVE_FLOW and (when configured) WFLOW only. Other supported ERC20 balances remain queryable via getUserPendingBalance(user, token) and getClaimableRefund(user, token).

Changes:

• Solidity: Return balanceTokens[], pendingBalances[], and claimableRefundsArray[] instead of single uint256 values
• Cadence: Update PendingRequestsInfo struct to use dictionaries pendingBalances: {String: UFix64} and claimableRefunds: {String: UFix64}
• Update ABI decoding to handle new array types
• Update all tests to use new return format

Test plan

• All 70 Solidity tests pass
• All 4 Cadence test suites pass
• Verify integration with frontend clients using the new return format

🤖 Generated with Claude Code

[claude]

Pull Request Review

Overview

This PR successfully addresses issue #30 by enhancing getPendingRequestsByUserUnpacked to return per-token balance arrays instead of single values. The implementation is clean and maintains backward compatibility through proper ABI versioning.

---

✅ Strengths

1. Clean API Design

• The new return format with parallel arrays (balanceTokens, pendingBalances, claimableRefundsArray) is intuitive and follows established patterns
• Dynamic array sizing based on WFLOW configuration is efficient (tokenCount = WFLOW != address(0) ? 2 : 1)
• Maintains O(1) complexity for user lookups

2. Cross-VM Consistency

• Solidity and Cadence implementations stay synchronized (solidity/src/FlowYieldVaultsRequests.sol:1425, cadence/contracts/FlowYieldVaultsEVM.cdc:1730)
• Proper ABI encoding/decoding with correct type specifications
• Dictionary-based approach in Cadence ({String: UFix64}) is idiomatic and extensible

3. Test Coverage

• All 70 Solidity tests updated to use new return format
• Tests cover key scenarios: multiple users, cancellations, processing, edge cases
• Proper assertions on array indices and balances

4. Documentation

• Updated FRONTEND_INTEGRATION.md with clear usage examples
• Inline comments explain the new return values
• Helpful note about fallback to individual getters for other ERC20s

---

⚠️ Issues & Recommendations

Critical: Missing Test Coverage

Issue: The ABI artifacts show new error types (YieldVaultTokenMismatch, YieldVaultTokenNotSet) and a new public mapping (yieldVaultTokens) that aren't part of this PR's stated changes.

// From deployments/artifacts/FlowYieldVaultsRequests.json
{
  "type": "function",
  "name": "yieldVaultTokens",
  "inputs": [{"name": "", "type": "uint64"}],
  "outputs": [{"name": "", "type": "address"}]
}

Recommendation:

• If these changes are from a previous commit that didn't update artifacts, that's fine
• If this is new functionality in this PR, it needs dedicated tests and should be mentioned in the PR description
• Run ./scripts/export-artifacts.sh to ensure artifacts match current contract state

Moderate: Array Index Safety

Location: cadence/contracts/FlowYieldVaultsEVM.cdc:1733

while j < balanceTokens.length {
    let tokenAddr = balanceTokens[j].toString()
    pendingBalances[tokenAddr] = FlowEVMBridgeUtils.uint256ToUFix64(value: pendingBalancesRaw[j], decimals: 18)
    // Assumes pendingBalancesRaw and claimableRefundsRaw have same length
    j = j + 1
}

Issue: No validation that pendingBalancesRaw.length == claimableRefundsRaw.length == balanceTokens.length

Recommendation: Add assertions before the loop:

assert(
    balanceTokens.length == pendingBalancesRaw.length && 
    balanceTokens.length == claimableRefundsRaw.length,
    message: "Balance array length mismatch in ABI decode"
)

Minor: Test Assertion Consistency

Location: solidity/test/FlowYieldVaultsRequests.t.sol:919

Some test destructuring statements have trailing commas in the middle of the parameter list:

(uint256[] memory ids, , , , , , , , , , address[] memory balanceTokens, uint256[] memory pendingBalances, ) = ...

Recommendation: While syntactically valid, consider naming intermediate parameters for readability:

(
    uint256[] memory ids,
    , // requestTypes
    , // statuses  
    , // tokenAddresses
    , // amounts
    ...
) = c.getPendingRequestsByUserUnpacked(user);

Minor: Gas Optimization Opportunity

Location: solidity/src/FlowYieldVaultsRequests.sol:1425-1440

uint256 tokenCount = WFLOW != address(0) ? 2 : 1;
balanceTokens = new address[](tokenCount);
pendingBalances = new uint256[](tokenCount);
claimableRefundsArray = new uint256[](tokenCount);

Observation: Conditional logic executed on every call. Since WFLOW is immutable, this could be optimized with early return branches, though current implementation is already quite efficient.

Verdict: Not worth optimizing - code clarity is more valuable here.

---

🔒 Security Analysis

✅ No Security Issues Found

• Read-only view function with no state modifications
• No reentrancy risks
• Array bounds properly handled by Solidity's automatic checks
• User-specific data isolation maintained through existing mappings
• No new privilege escalation vectors

---

🎯 Recommendations

Before Merge:

1. Clarify Artifact Changes - Confirm whether yieldVaultTokens, YieldVaultTokenMismatch, and YieldVaultTokenNotSet are from this PR or a previous commit
2. Add Array Length Validation - Add assertions in Cadence decoder to prevent index out-of-bounds
3. Update Test Plan Checkbox - "Verify integration with frontend clients" is unchecked - either test or remove

Optional Improvements:

4. Consider adding a test that explicitly verifies WFLOW balances when WFLOW is configured
5. Add a test for the edge case where a user has pending balance in WFLOW but not NATIVE_FLOW (or vice versa)

---

📊 Final Assessment

Code Quality: ⭐⭐⭐⭐½ (4.5/5)
Test Coverage: ⭐⭐⭐⭐ (4/5)
Security: ⭐⭐⭐⭐⭐ (5/5)
Documentation: ⭐⭐⭐⭐ (4/5)

Verdict: ✅ Approve with minor fixes

This is a solid implementation that correctly solves the stated problem. The cross-VM synchronization is well-executed, and the API design is extensible. The identified issues are minor and don't block merge, but addressing the array validation would improve robustness.

Great work maintaining consistency across Solidity and Cadence! The frontend integration guide is particularly helpful.

---

🤖 Review generated by Claude Code

[claude]

Hardcoded decimals: 18 will silently mis-represent any ERC-20 token that does not have 18 decimal places

Both lines here use decimals: 18:

pendingBalances[tokenAddr] = FlowEVMBridgeUtils.uint256ToUFix64(value: pendingBalancesRaw[j], decimals: 18)
claimableRefundsMap[tokenAddr] = FlowEVMBridgeUtils.uint256ToUFix64(value: claimableRefundsRaw[j], decimals: 18)

NATIVE_FLOW and WFLOW both have 18 EVM decimals, so this PR is correct for the current token set. However, the Solidity side already has a generic setTokenConfig admin function that can add any ERC-20 token, and the balance arrays are keyed by token address — if a 6-decimal USDC or similar token were ever added, the Cadence side would silently report values ~10¹² times too small.

The safe fix is to use FlowEVMBridgeUtils.convertERC20AmountToCadenceAmount (which looks up the actual decimals via FlowEVMBridgeConfig) for non-native tokens, matching what ufix64FromUInt256 already does elsewhere in this contract. Alternatively, add a hard assertion that only NATIVE_FLOW/WFLOW (18-decimal) addresses can appear in this loop, so the assumption is at least explicit and checked.

[claude]

The WFLOW balance path (index [1]) has no test coverage — the core feature of this fix is untested

Every updated test assertion in this PR checks pendingBalances[0] / claimableRefundsArr[0] (NATIVE_FLOW). No test exercises:

• balanceTokens[1] == WFLOW when WFLOW is configured
• pendingBalances[1] reflecting an active WFLOW CREATE/DEPOSIT request
• claimableRefundsArr[1] reflecting a cancelled/dropped WFLOW request

A test along these lines is needed:

1. Approve WFLOW, call createYieldVault(WFLOW, 2 ether, ...) — verify pendingBalances[1] == 2 ether.
2. Cancel the request — verify pendingBalances[1] == 0 and claimableRefundsArr[1] == 2 ether.

Without this test the regression this PR aims to fix (WFLOW balances not returned) could silently re-appear.

[claude]

Review: fix(FLOW-15) — Return per-token balances in getPendingRequestsByUserUnpacked

The change is logically correct and well-structured. The Solidity, Cadence, ABI artifacts, and frontend docs are all updated in sync. Two concrete issues worth fixing before merge:

---

1. Missing test for the WFLOW = address(0) case (correctness gap)

The new code forks on WFLOW != address(0) to return either a 1- or 2-element array:

uint256 tokenCount = WFLOW != address(0) ? 2 : 1;

setUp() now always configures WFLOW (via the new ERC20Mock), so the single-element path is never exercised by any test. A client that writes balanceTokens[1] will revert on a deployment without WFLOW. A test that constructs the contract with address(0) and asserts balanceTokens.length == 1 pins the invariant.

---

2. test_GetPendingRequestsByUserUnpacked_SingleUser is under-specified (weak coverage)

The test captures 12 of the 13 return values — claimableRefundsArray is silently dropped via trailing comma, and only balanceTokens[0] / pendingBalances[0] are asserted. Since WFLOW is always configured, the call returns a 2-element array but the test never checks:

• balanceTokens.length == 2
• pendingBalances[1] == 0 (no WFLOW pending)
• claimableRefundsArray[0] == 0 (no refund yet)

The new test_GetPendingRequestsByUserUnpacked_WFLOWBalances test is much more thorough; the single-user test should at minimum assert the array length.

---

No other material issues found

• Cadence PendingRequestsInfo struct change (scalar → dictionary) is correct and no existing Cadence tests reference the removed fields.
• Cadence ABI decode type hints ([EVM.EVMAddress], [UInt256]) match the updated Solidity return types.
• The assert on array length equality before iteration is a good defensive addition.
• ufix64FromUInt256 (access(self)) is called correctly from within the contract body.
• Unit conversion path for WFLOW (convertERC20AmountToCadenceAmount) is consistent with existing request-processing code.

[claude]

The test setup now always configures WFLOW, so balanceTokens.length will be 2 here. The test only checks index 0 and silently drops claimableRefundsArray (trailing comma). Consider asserting balanceTokens.length == 2, pendingBalances[1] == 0 (no WFLOW pending), and that claimableRefundsArray[0] == 0 to make the return shape fully exercised for the WFLOW-configured case.

[claude]

There is no test covering the WFLOW = address(0) path. In that case tokenCount == 1 and callers receive a single-element array. Any client that hardcodes balanceTokens[1] (easy to write after reading the WFLOW example) will out-of-bounds revert on a deployment that has no WFLOW. A companion test that constructs the contract with WFLOW = address(0) and asserts balanceTokens.length == 1 would pin this invariant.