fix(FLOW-10): Enforce fair FIFO queue ordering on-chain

[m-Peter]

Closes #25

FLOW-10: FIFO Queue Is Not Enforced on-Chain yet Costs O(n) to Maintain

Introduce an optimized queue data structure (mapping-based with head & tail pointers), to avoid the high gas costs of maintaining the FIFO order on-chain. Both enqueue & dequeue operations are now O(1).

The previous queue data structure required an array and a mapping, and had O(n) performance when removing a request from the pending queue:

/// @notice Array of pending request IDs awaiting processing (FIFO order)
uint256[] public pendingRequestIds;

/// @notice Index of request ID in global pending array (for O(1) lookup)
mapping(uint256 => uint256) private _requestIndexInGlobalArray;

In additionn _startProcessingInternal now has a check which verifies that the request being processed is the head of the requestsQueue FIFO queue:

_removeUserPendingRequest(requestId);
uint256 reqId = _dequeueRequest();
if (reqId != requestId) revert RequestProcessOutOfOrder(reqId, requestId);

If that is not the case, the function call reverts with RequestProcessOutOfOrder.

The Cadence Scheduler that schedules & processes the requests, is fetching the request IDs with:

worker.getPendingRequestsFromEVM(startIndex: 0, count: fetchCount)

which returns a given count of pending EVM requests from the queue, in FIFO order.
These are fed to preprocessRequests(), and after the validation checks, they are classified as successful/rejected, and they are then passed in to startProcessingBatch(), which drops the rejected request IDs, and calls _startProcessingInternal() for each individual successful request ID.

[claude]

Code Review: Enforce Fair FIFO Queue Ordering On-Chain

Summary

This PR addresses FLOW-10 by replacing the O(n)-shift pendingRequestIds array with a mapping-based queue (head/tail pointers), and enforcing FIFO ordering on-chain via _dequeueRequest() inside _startProcessingInternal. The COA can no longer skip a successful request — any out-of-order attempt reverts with RequestProcessOutOfOrder. The overall approach is sound and the implementation is correct.

Strengths

• Core security goal achieved: _startProcessingInternal calls _dequeueRequest() and validates the dequeued ID matches the requested ID. Combined with EVM transaction atomicity, mismatched IDs cause a full revert with no partial state changes.
• O(1) enqueue/dequeue for the common path: The mapping + head/tail pointer design is a well-known gas-efficient pattern and a genuine improvement over the old O(n) array-shift for successful processing.
• _dropQueuedRequest is correctly implemented: The loop correctly shifts elements left from the found position, deletes the now-duplicated tail element, and only decrements _requestsQueueTail when shiftQueue is true. No bugs here.
• Good test coverage: New tests cover single-request revert, multi-request batch (all success, all reject, mixed), out-of-order reverts, and random cancellation patterns.

Important Issues

1. Cadence worker not updated — will cause production reverts

The Solidity contract now requires successful requests to be submitted to startProcessingBatch in strict FIFO queue order. However, neither FlowYieldVaultsEVM.cdc nor FlowYieldVaultsEVMWorkerOps.cdc has been updated in this PR. If the Cadence worker currently selects or orders requests by any logic other than queue-head-first, every call to startProcessingBatch will revert with RequestProcessOutOfOrder.

This is the highest-priority gap to close — either update the Cadence code in this PR, or document the required Cadence changes and block merging on their completion.

2. FIFO bypass for rejections remains possible

startProcessingBatch enforces FIFO for successfulRequestIds (each validated against the queue head), but rejectedRequestIds are removed via _dropQueuedRequest which can target any queue position. A COA operator could achieve non-FIFO outcomes by classifying early requests as rejected — e.g., rejecting req1-req3 and processing req4-req5 skips ahead in the queue.

This may be intentional — genuinely invalid requests should not block valid ones — but the NatDoc comment added in this PR is a trust assumption, not an on-chain guarantee. Consider making the intended threat model explicit.

Minor Issues

3. requestsQueue visibility could be private

requestsQueue is declared internal, making it accessible to inheriting contracts and allowing queue invariants to be bypassed. If no inheritance is intended, private would be safer.

4. _dropQueuedRequest silently no-ops for unknown IDs

If requestId is not found in the queue, shiftQueue stays false and the function returns without any revert or event. Callers validate state first, so this is safe in practice, but a comment explaining why silent failure is acceptable here would improve maintainability.

5. Performance claim slightly overstates improvement

_dequeueRequest is now O(1), which is a genuine win. However _dropQueuedRequest (used for cancellations and batch rejections) is still O(n) — the same asymptotic cost as the old array-shift approach, though with fewer SSTORE operations. This is unavoidable for arbitrary-position removal in a FIFO queue, but worth noting to set correct expectations.

Test Observations

• test_StartProcessingBatch_MultipleRejectedRequestsOutOfOrder correctly documents that rejected IDs can be in any order — a useful property to test explicitly.
• test_CancelRequests_RandomOrder comprehensively covers head, tail, and middle cancellation.
• The existing test_FIFOOrdering_WithRemoval was correctly updated to reflect the new strict FIFO enforcement.

Generated with Claude Code