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]

PR Review: fix(FLOW-10) — Enforce Fair FIFO Queue Ordering On-Chain

Overall this is a solid improvement. The mapping-based queue eliminates the costly O(n) array shift for normal processing, and the RequestProcessOutOfOrder guard adds a meaningful correctness guarantee. A few issues deserve attention before merging.

---

Bug / Correctness

RequestProcessOutOfOrder error arguments are swapped relative to their semantic meaning

In _startProcessingInternal (line ~1529–1530):

uint256 reqId = _dequeueRequest();          // head of queue
if (reqId != requestId) revert RequestProcessOutOfOrder(reqId, requestId);

The error is declared as:

error RequestProcessOutOfOrder(uint256 expectedId, uint256 processedId);

reqId is the head of the queue (the expected next request) and requestId is what the COA supplied (the processed one). The argument order here is therefore (expected, processed) which matches the declaration — so this is correct. However, the test test_StartProcessingBatch_MultipleSuccessfulRequestsRevertRequestProcessOutOfOrder sets:

vm.expectRevert(abi.encodeWithSelector(
    FlowYieldVaultsRequests.RequestProcessOutOfOrder.selector,
    req2, // "the expected requestId, in the queue head"
    req4  // "the provided requestId"
));

After rejecting req1, req3, req5, the remaining queue head is req2, and the first successful ID passed is req4. That means reqId = req2 and requestId = req4, so the revert fires with (req2, req4). The test expectation is (req2, req4). ✓

No actual bug here, but the comment in the test is misleading about what "head of the queue" means after rejections have already been removed. Worth a clarifying comment.

---

Performance / Gas

_dropQueuedRequest is still O(n) — this should be documented

The PR title and description correctly state that enqueue and dequeue are O(1), but _dropQueuedRequest (used for cancellations and for every request in rejectedRequestIds) still scans and shifts the entire queue from the removal point to the tail. For a queue of length n:

• Each _dropQueuedRequest call: O(n)
• A batch with k rejections: O(k × n)

In the worst case (large queue, many rejections per batch), this can be expensive. The PR description does not mention this residual O(n) cost, which could be misleading to future readers.

Suggestion: Add a NatSpec note to _dropQueuedRequest explicitly documenting its O(n) complexity and why it is unavoidable for mid-queue removal:

/// @dev O(n) operation — scans from the removed element to the tail and shifts
///      all subsequent elements left to maintain FIFO order.

---

Code Style / Naming

requestsQueue is missing the leading underscore used by all other private fields

Every other private/internal field in the contract uses a leading _:

uint256 private _requestsQueueHead = 1;
uint256 private _requestsQueueTail = 1;
mapping(address => mapping(uint64 => uint256)) internal _yieldVaultIndexInUserArray;
mapping(uint256 => uint256) private _requestIndexInUserArray;

But the new queue mapping is declared without it:

mapping(uint256 => uint256) private requestsQueue;   // inconsistent

This should be _requestsQueue for consistency.

---

Documentation

_requestsQueueTail NatSpec comment is misleading

/// @notice Pointer to the current tail in requestsQueue. Denotes the last request to be processed
uint256 private _requestsQueueTail = 1;

_requestsQueueTail is an exclusive upper bound — it points to the slot after the last element, not to the last element itself (evidenced by _enqueueRequest writing to requestsQueue[_requestsQueueTail] then incrementing). The description "Denotes the last request to be processed" is incorrect.

Suggested fix:

/// @notice Pointer to the current tail in requestsQueue. Points to the next available
///         slot — i.e., one past the last enqueued request.

---

Test Coverage

The new test suite is comprehensive and covers the key scenarios well:

• Out-of-order successful processing reverts ✓
• All-successful, all-rejected, and mixed batches ✓
• Cancellations in random order ✓
• FIFO ordering assertions post-cancellation ✓

One scenario worth adding: duplicate request IDs across successfulRequestIds and rejectedRequestIds in the same startProcessingBatch call. Currently, a duplicate would first mark the request FAILED (via _dropRequestsInternal), then hit InvalidRequestState in _startProcessingInternal, reverting the whole batch. This is correct behavior, but an explicit test would document the invariant.

---

Summary

Severity	Issue	Location
Style	requestsQueue missing _ prefix	FlowYieldVaultsRequests.sol:~170
Docs	_requestsQueueTail comment says "last to be processed" but is exclusive upper bound	FlowYieldVaultsRequests.sol:~176
Docs	_dropQueuedRequest O(n) complexity not documented	FlowYieldVaultsRequests.sol:~1768
Test	Missing coverage for duplicate IDs in successful+rejected arrays	FlowYieldVaultsRequests.t.sol

The core logic is correct and the implementation achieves its stated goal. The style and documentation issues are minor; the O(n) complexity of _dropQueuedRequest is worth calling out explicitly so it doesn't surprise future maintainers.