fix(FLOW-15): Return per-token balances in getPendingRequestsByUserUnpacked#36
Conversation
…packed
Previously, `getPendingRequestsByUserUnpacked` only returned native FLOW
balances, ignoring ERC20/WFLOW balances. This made it impossible for
clients to see pending balances for non-native tokens.
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
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Pull Request ReviewOverviewThis PR successfully addresses issue #30 by enhancing ✅ Strengths1. Clean API Design
2. Cross-VM Consistency
3. Test Coverage
4. Documentation
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| var j = 0 | ||
| while j < balanceTokens.length { | ||
| let tokenAddr = balanceTokens[j].toString() | ||
| pendingBalances[tokenAddr] = FlowEVMBridgeUtils.uint256ToUFix64(value: pendingBalancesRaw[j], decimals: 18) |
There was a problem hiding this comment.
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.
| if (WFLOW != address(0)) { | ||
| balanceTokens[1] = WFLOW; | ||
| pendingBalances[1] = pendingUserBalances[user][WFLOW]; | ||
| claimableRefundsArray[1] = claimableRefunds[user][WFLOW]; |
There was a problem hiding this comment.
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] == WFLOWwhen WFLOW is configuredpendingBalances[1]reflecting an active WFLOW CREATE/DEPOSIT requestclaimableRefundsArr[1]reflecting a cancelled/dropped WFLOW request
A test along these lines is needed:
- Approve WFLOW, call
createYieldVault(WFLOW, 2 ether, ...)— verifypendingBalances[1] == 2 ether. - Cancel the request — verify
pendingBalances[1] == 0andclaimableRefundsArr[1] == 2 ether.
Without this test the regression this PR aims to fix (WFLOW balances not returned) could silently re-appear.
Review: fix(FLOW-15) — Return per-token balances in
|
| , | ||
| address[] memory balanceTokens, | ||
| uint256[] memory pendingBalances, | ||
| uint256[] memory claimableRefundsArr |
There was a problem hiding this comment.
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.
| uint256[] memory claimableRefundsArr | ||
| ) = c.getPendingRequestsByUserUnpacked(user); | ||
|
|
||
| assertEq(ids.length, 1, "User should have 1 pending request"); |
There was a problem hiding this comment.
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.
|
|
||
| if (!_isTrackedToken[tokenAddress]) { | ||
| _isTrackedToken[tokenAddress] = true; | ||
| _trackedTokens.push(tokenAddress); |
There was a problem hiding this comment.
_trackedTokens grows monotonically with no removal path.
Once a token is pushed here it stays forever, even after the owner disables it and every user's pending/refund balance for it reaches zero. In that state every call to getPendingRequestsByUserUnpacked returns a row of all-zeroes for the stale token indefinitely, and there is no admin function to prune it.
The PR description notes "may include disabled tokens if users still have balances/refunds", but the current design keeps the token even after balances are fully claimed. Consider adding an admin function removeTrackedToken(address) that requires pendingUserBalances and claimableRefunds to be zero for that token across all users (or at least a best-effort guard), so the list can be cleaned up over time.
There was a problem hiding this comment.
We won't support more then 20 tokens so it seems acceptable not to adda remove API here @claude
There was a problem hiding this comment.
Claude finished @liobrasil's task in 24s —— View job
That's a reasonable constraint. For future maintainability, consider adding a short inline comment at the _trackedTokens.push site and a guard that makes the limit explicit:
// Max 20 tokens are ever expected; guard prevents unbounded array growth.
require(_trackedTokens.length < 20, "Max tracked token limit reached");
_trackedTokens.push(tokenAddress);This turns the "won't support" policy into a hard on-chain invariant rather than relying solely on admin discipline — and keeps the concern from resurfacing in future reviews.
I'll withdraw the removal-API concern. Up to you whether you want the guard.
| let balanceTokens = decoded[10] as! [EVM.EVMAddress] | ||
| let pendingBalancesRaw = decoded[11] as! [UInt256] | ||
| let claimableRefundsRaw = decoded[12] as! [UInt256] | ||
|
|
||
| // Convert pending balance from wei to UFix64 | ||
| let pendingBalance = FlowEVMBridgeUtils.uint256ToUFix64(value: pendingBalanceRaw, decimals: 18) | ||
| let claimableRefund = FlowEVMBridgeUtils.uint256ToUFix64(value: claimableRefundRaw, decimals: 18) | ||
| assert( | ||
| balanceTokens.length == pendingBalancesRaw.length | ||
| && balanceTokens.length == claimableRefundsRaw.length, | ||
| message: "Balance array length mismatch in ABI decode" | ||
| ) | ||
|
|
||
| // Build per-token balance dictionaries | ||
| var pendingBalances: {String: UFix64} = {} | ||
| var claimableRefundsMap: {String: UFix64} = {} | ||
| var j = 0 | ||
| while j < balanceTokens.length { | ||
| let tokenAddr = balanceTokens[j].toString() | ||
| pendingBalances[tokenAddr] = FlowYieldVaultsEVM.ufix64FromUInt256( | ||
| pendingBalancesRaw[j], | ||
| tokenAddress: balanceTokens[j] | ||
| ) | ||
| claimableRefundsMap[tokenAddr] = FlowYieldVaultsEVM.ufix64FromUInt256( | ||
| claimableRefundsRaw[j], | ||
| tokenAddress: balanceTokens[j] | ||
| ) | ||
| j = j + 1 | ||
| } |
There was a problem hiding this comment.
The new ABI-decode path has no Cadence-side test coverage.
This block is the most complex change in the PR: it decodes three new return values ([EVM.EVMAddress], [UInt256], [UInt256]), iterates them, and calls ufix64FromUInt256 (which itself dispatches on whether the token is native FLOW or an ERC20). A wrong type in the decoded[N] as! cast causes a runtime panic; a wrong decimal conversion silently produces incorrect balances.
All four Cadence test suites pass trivially because none of them call getPendingRequestsForEVMAddress. The get_pending_requests_for_evm_address.cdc script and the code path below are completely unexercised by the Cadence test suite. At minimum, evm_bridge_lifecycle_test.cdc should assert that the returned PendingRequestsInfo.pendingBalances dictionary contains the expected token keys and converted values after a request is created.
| uint256 pendingBalance, | ||
| address[] memory balanceTokens, | ||
| uint256[] memory pendingBalances, | ||
| ) = c.getPendingRequestsByUserUnpacked(user); |
There was a problem hiding this comment.
The trailing comma silently drops the claimableRefundsArray return value, so this test has no assertion on it. With two pending NATIVE_FLOW requests and no cancellation, claimableRefundsArray[0] should be 0; the test should verify that.
| ) = c.getPendingRequestsByUserUnpacked(user); | |
| uint256[] memory pendingBalances, | |
| uint256[] memory claimableRefundsArray | |
| ) = c.getPendingRequestsByUserUnpacked(user); |
Then add:
assertEq(claimableRefundsArray[0], 0, "No refund expected for active pending requests");
|
|
||
| assertEq(ids.length, 0, "New user should have no pending requests"); | ||
| assertEq(pendingBalance, 0, "New user should have 0 pending balance"); | ||
| assertEq(pendingBalances[0], 0, "New user should have 0 pending balance"); |
There was a problem hiding this comment.
This assertion checks only pendingBalances[0] (NATIVE_FLOW). If _trackedTokens were empty for some reason this would OOB-revert rather than produce a clean failure message. More usefully: the test doesn't assert that balanceTokens.length == 2 (NATIVE_FLOW + WFLOW are both tracked from the constructor), nor that pendingBalances[1] == 0 and all claimableRefundsArray slots are zero. For a test called "EmptyForNewUser" it should confirm the full balance picture is zeroed, not just one slot.
|
|
||
| if (!_isTrackedToken[tokenAddress]) { | ||
| _isTrackedToken[tokenAddress] = true; | ||
| _trackedTokens.push(tokenAddress); |
There was a problem hiding this comment.
_trackedTokens grows permanently with no cap
Every distinct address ever passed to _setTokenConfig (directly via setTokenConfig or by the constructor) is appended once and never pruned. Disabling a token (isSupported = false) keeps it in the list.
The Cadence balance-query loop then issues one decimals() EVM call per non-native tracked token per query call. Over the contract's lifetime, an admin adding many tokens (even short-lived test/staging tokens) permanently inflates this cost for every future call.
Consider documenting an explicit governance rule (e.g., "the tracked-token list is expected to remain small — typically ≤ 10 entries") or adding an owner-controlled removeTrackedToken escape hatch so dead tokens can be pruned if needed. At minimum, a getTrackedTokens() view would make the current list inspectable without reading contract storage directly.
|
|
||
| assertEq(ids.length, 0, "New user should have no pending requests"); | ||
| assertEq(pendingBalance, 0, "New user should have 0 pending balance"); | ||
| assertEq(pendingBalances[0], 0, "New user should have 0 pending balance"); |
There was a problem hiding this comment.
Fragile index access: pendingBalances[0] without length guard
This works today because the constructor always calls _setTokenConfig(NATIVE_FLOW, ...), ensuring _trackedTokens.length >= 1. But the assertion doesn't verify that precondition — if _trackedTokens were somehow empty (e.g., a future constructor variant, a different test helper), this would revert rather than fail gracefully.
Suggested addition before line 1205:
assertGt(pendingBalances.length, 0, "Balance array should be non-empty (at least NATIVE_FLOW)");
|
|
||
| if (isSupported && !_isTrackedToken[tokenAddress]) { | ||
| _isTrackedToken[tokenAddress] = true; | ||
| _trackedTokens.push(tokenAddress); |
There was a problem hiding this comment.
_trackedTokens is append-only — no removal path exists.
Once a token enters _trackedTokens, it stays forever, even if the owner later wants to drop it entirely (e.g., a misconfigured address, a migrated token contract, or a deprecated asset). setTokenConfig(token, false, ...) disables the token for new deposits but does not remove it from this array, so it keeps showing up as a zero-balance slot in every getPendingRequestsByUserUnpacked response.
In practice the list is expected to stay small (2–5 tokens), but the design gives no escape hatch if a token ever needs to be fully deregistered. Consider adding an owner-only untrackToken(address) that:
- clears
_isTrackedToken[token] - swaps-and-pops the address out of
_trackedTokens
This also lets you enforce a cap on _trackedTokens.length if desired, preventing a theoretical scenario where a large list causes the Cadence dryCall (15 M gas limit) to time out.
| "name": "pendingBalance", | ||
| "type": "uint256", | ||
| "internalType": "uint256" | ||
| "name": "balanceTokens", |
There was a problem hiding this comment.
The solidity/deployments/artifacts/FlowYieldVaultsRequests.json artifact is out of sync with deployments/artifacts/FlowYieldVaultsRequests.json. The top-level copy (which is what scripts/export-artifacts.sh actually generates) includes three entries added in this PR that are absent here:
DEFAULT_MINIMUM_BALANCE(view function)CannotBlocklistZeroAddress(custom error)InvalidMinimumBalance(custom error)
All three are present in the Solidity source. Any consumer using solidity/deployments/artifacts/ will have an incomplete ABI and won't be able to catch or decode these errors.
The export script only writes to deployments/artifacts/, so this secondary copy needs to be kept in sync manually or the script should also update it.
| isNative: isNative | ||
| }); | ||
|
|
||
| if (isSupported && !_isTrackedToken[tokenAddress]) { |
There was a problem hiding this comment.
_trackedTokens is append-only: once a token is pushed it is never removed, even after the token is disabled and all user balances/refunds have been claimed. Over the life of the contract, every token that was ever enabled at least once will appear in the array permanently. This means getPendingRequestsByUserUnpacked iterates over — and returns — an ever-growing set of token entries for every user query, regardless of whether the user ever interacted with those tokens.
For the current governance-controlled usage (a handful of tokens) this is low risk, but there is no escape hatch (e.g. pruneTrackedToken(address) callable when both pendingUserBalances and claimableRefunds are zero for all users) if the list grows large. Worth at least a @dev note on the field explaining the one-way growth.
| while j < balanceTokens.length { | ||
| let tokenAddr = balanceTokens[j].toString() | ||
| let rawPending = pendingBalancesRaw[j] | ||
| let rawRefund = claimableRefundsRaw[j] | ||
| pendingBalances[tokenAddr] = rawPending == 0 | ||
| ? 0.0 | ||
| : FlowYieldVaultsEVM.ufix64FromUInt256( | ||
| rawPending, | ||
| tokenAddress: balanceTokens[j] | ||
| ) | ||
| claimableRefundsMap[tokenAddr] = rawRefund == 0 | ||
| ? 0.0 | ||
| : FlowYieldVaultsEVM.ufix64FromUInt256( | ||
| rawRefund, | ||
| tokenAddress: balanceTokens[j] | ||
| ) |
There was a problem hiding this comment.
New failure mode: ufix64FromUInt256 panics if a tracked ERC20's decimals() is unreachable
ufix64FromUInt256 on a non-native-FLOW token calls FlowEVMBridgeUtils.convertERC20AmountToCadenceAmount, which issues a cross-VM decimals() call. If that call reverts (e.g. the token contract was self-destructed, or a non-ERC20 address was ever mistakenly registered and given balances), the entire getPendingRequestsForEVMAddress script panics for every user who holds a non-zero balance for that token.
Before this PR the Cadence query decoded a single scalar for native FLOW and made no ERC20 decimals() calls. The new loop iterates over every tracked token and queries decimals for any non-zero amount, so this failure mode is new to this PR.
The zero-shortcut (rawPending == 0 ? 0.0 : …) correctly avoids the call for empty slots, but it does not protect users who have live escrow or claimable refunds in a now-defunct token.
| access(all) | ||
| fun testGetPendingRequestsForEVMAddressDecodesBalances() { | ||
| let tokenBAddress = deployERC20DecimalsOnlyMock(admin, decimals: 6) | ||
| let tokenCAddress = "0x00000000000000000000000000000000000000cc" |
There was a problem hiding this comment.
Test gap: tokenC never exercises the non-zero ERC20 decimal-lookup path
tokenCAddress is a bare address with no deployed contract. The mock (PendingRequestsByUserQueryMock) leaves pendingBalances[2] and claimableRefundsArray[2] at their default of 0, so the rawPending == 0 ? 0.0 : ufix64FromUInt256(…) shortcut in FlowYieldVaultsEVM.cdc means decimals() is never actually called for that address.
This means the risky path—"tracked token with a non-zero amount but an unresponsive ERC20 contract"—has no test coverage at all. Consider adding a second variant of the mock that sets a non-zero balance for tokenC with a non-deployed address to confirm the expected behavior (panic with a clear message, or a graceful fallback).
| if (isSupported && !_isTrackedToken[tokenAddress]) { | ||
| _isTrackedToken[tokenAddress] = true; | ||
| _trackedTokens.push(tokenAddress); |
There was a problem hiding this comment.
_trackedTokens is unbounded and irreversible
_isTrackedToken is set to true once and never cleared, and _trackedTokens is append-only. Every subsequent call to setTokenConfig with a fresh address adds another permanent entry. There is no admin function to prune stale entries.
On the Solidity side this inflates the return array of getPendingRequestsByUserUnpacked linearly with the number of ever-enabled tokens. On the Cadence side, each non-zero entry in that array triggers an additional cross-VM decimals() call during getPendingRequestsForEVMAddress. In a long-running deployment with rotating token lists this can grow without bound.
Consider documenting the intended upper limit, or adding an owner-only removeTrackedToken that is allowed only when all user balances for that token are zero.
| access(all) let pendingBalance: UFix64 | ||
| access(all) let claimableRefund: UFix64 | ||
| /// @notice Pending balances per token address (maps token address string -> UFix64 balance) | ||
| access(all) let pendingBalances: {String: UFix64} |
There was a problem hiding this comment.
Since the token address is basically EVM.EVMAddress, can we use it as key instead?
If not, I would suggest to mention in the comments something like the token address string is the hex format of EVM.EVMAddress.
There was a problem hiding this comment.
I did some checks and {EVM.EVMAddress: UFix64} is not viable in Cadence. EVM.EVMAddress is a struct, and Cadence does not allow struct types as dictionary keys.
…20260407 # Conflicts: # cadence/tests/test_helpers.cdc
| "name": "pendingBalance", | ||
| "type": "uint256", | ||
| "internalType": "uint256" | ||
| "name": "balanceTokens", |
There was a problem hiding this comment.
This artifact copy is out of sync with the canonical deployments/artifacts/FlowYieldVaultsRequests.json. Three ABI entries present in the canonical file are missing here:
DEFAULT_MINIMUM_BALANCE(function)CannotBlocklistZeroAddress(error)InvalidMinimumBalance(error)
scripts/export-artifacts.sh only writes to deployments/artifacts/; solidity/deployments/artifacts/ appears to be maintained manually and was only partially updated in this PR. Either regenerate this file from the Foundry output (or the canonical file) to keep them in sync, or remove the duplicate copy and point all consumers to the canonical path.
| pendingBalances[tokenAddr] = rawPending == 0 | ||
| ? 0.0 | ||
| : FlowYieldVaultsEVM.ufix64FromUInt256( | ||
| rawPending, | ||
| tokenAddress: balanceTokens[j] | ||
| ) | ||
| claimableRefundsMap[tokenAddr] = rawRefund == 0 | ||
| ? 0.0 | ||
| : FlowYieldVaultsEVM.ufix64FromUInt256( | ||
| rawRefund, | ||
| tokenAddress: balanceTokens[j] | ||
| ) |
There was a problem hiding this comment.
The zero-guard correctly skips the decimals() EVM call for zero balances, but any non-zero ERC20 balance/refund entry will call FlowEVMBridgeUtils.convertERC20AmountToCadenceAmount, which panics if the ERC20's decimals() reverts (non-standard token, self-destructed contract, etc.).
Before this PR, ufix64FromUInt256 was only called inside the request loop for request.amount — so the query only panicked if there were pending requests for the broken token. Now it also fires on non-zero claimable refund entries, even when the user has no pending requests. A user who was refunded from a later-broken ERC20 would have getPendingRequestsForEVMAddress permanently stuck.
The SchedulerHandler is unaffected (it uses getPendingRequestsFromEVM → getPendingRequestsUnpacked, not this path), so worker processing is safe. However, monitoring scripts and any frontend caller hitting this endpoint would be unable to query that user's data.
Worth adding a defensive try/catch around the conversion — fall back to 0.0 with a logged event, rather than panicking the whole query:
// pseudo-pattern
let converted: UFix64 = rawPending == 0 ? 0.0 : (
FlowYieldVaultsEVM._safeUfix64FromUInt256(rawPending, tokenAddress: balanceTokens[j]) ?? 0.0
)Or at minimum add a comment noting this invariant so future token additions are made with it in mind.
3 participants
Summary
Fixes #30
getPendingRequestsByUserUnpackednow returns per-token pending and refund balances for the full tracked token set instead of only native FLOW. The unpacked balance view keeps previously configured tokens visible so users can still see residual balances or claimable refunds even after a token is later disabled.Changes
pendingBalance/claimableRefundreturn values with alignedbalanceTokens[],pendingBalances[], andclaimableRefundsArray[]_trackedTokenssoNATIVE_FLOW,WFLOW, and supported ERC20s are surfaced in the unpacked viewPendingRequestsInfo.pendingBalancesandPendingRequestsInfo.claimableRefundsUFix64WFLOW, tracked ERC20 balances, disabled-token visibility, and never-supported token exclusionTest plan