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]

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

The mapping-based queue (head/tail pointers) is the right data structure for O(1) enqueue/dequeue, and the FIFO enforcement direction is correct. Four concrete issues worth addressing before merge:

---

1. Undocumented ordering constraint on both input arrays (High)

startProcessingBatch requires that both successfulRequestIds and rejectedRequestIds individually be in FIFO queue order (each must be an ordered subsequence of the queue starting at head). If either array has elements out of queue order the function reverts with RequestProcessOutOfOrder.

This constraint is nowhere in the NatSpec. The Cadence preprocessRequests must preserve FIFO order across both output arrays; any sort or regroup in Cadence will cause all batch transactions to revert silently. Add a @dev note documenting this invariant. → line 968

---

2. RequestProcessOutOfOrder error parameters are inconsistent between call sites (Medium)

• Line 967: RequestProcessOutOfOrder(requestId, reqId) — reqId is the last non-matching ID tried from either input array, not the "processedId" the error name implies.
• Line 1573: RequestProcessOutOfOrder(reqId, requestId) — reqId is the dequeued head, requestId is the caller-supplied value. Semantically clear.

Callers and off-chain monitors decoding the error from line 967 will get a second argument with no obvious meaning. Either document the parameter semantics on the error definition, or align both call sites to the same (expected, actual) convention. → line 1573

---

3. Silent skip in _dropRequestsInternal can leave stale queue entries (Medium)

_dropRequestsInternal only calls _dropQueuedRequest if request.status == RequestStatus.PENDING. If that guard fails (request somehow not PENDING), the queue entry is left intact while accounting state is not updated — stalling all subsequent dequeues. The validation loop immediately before confirmed these IDs occupy specific queue slots, so the invariant should hold; but a silent skip masks the inconsistency instead of reverting. An explicit revert here would surface queue/state divergence immediately rather than silently corrupting the queue. → line 1471

---

4. Batch rejection is still O(k×n), not O(1) (Low / documentation)

_dropQueuedRequest is O(queue length) — correctly noted in its own docstring. Each rejected request in _dropRequestsInternal triggers one O(n) scan, making a batch with k rejections O(k×n). For spam bursts where most requests are rejected, this can exceed the old array approach. The PR description claims "both enqueue & dequeue are now O(1)" which is accurate for the normal processing path, but the summary should acknowledge that cancellations and rejections remain O(n). → line 1915

---

5. Missing negative test for out-of-order input arrays (Low)

There is no test asserting that passing successfulRequestIds or rejectedRequestIds out of FIFO order reverts. Given this is the core correctness invariant of the PR, it warrants an explicit regression test. → test line 394

[claude]

Undocumented ordering constraint + misleading error payload

The validation loop enforces that both successfulRequestIds and rejectedRequestIds must be in FIFO queue order — i.e., they must individually form sorted subsequences of the queue. If either array lists requests out of queue order, the function reverts here.

This constraint is not documented anywhere in the NatSpec. The Cadence preprocessRequests must preserve FIFO order in each output array; if it ever groups or sorts by any other criterion, all batch transactions will spuriously revert.

The error payload is also confusing: RequestProcessOutOfOrder(requestId, reqId) where reqId is the last non-matching ID tried (either from successfulRequestIds[j] or rejectedRequestIds[k] — whichever was tested last), not the "processedId" the error name implies. A caller decoding the error will see a value that has no obvious relationship to the request being processed.

Suggested fixes:

1. Add @dev NatSpec: both arrays must be in ascending FIFO queue order (i.e., must form ordered subsequences of the current queue starting at head).
2. For the error, capture which input array and index caused the mismatch, or simplify to just emit the expected queue-head value.

[claude]

RequestProcessOutOfOrder parameter order is reversed vs. line 967

Here the error is (reqId, requestId) — (dequeued_head, caller_requested) — which reads "expected reqId, got requestId". That's semantically clear.

At line 967, the same error is emitted as (requestId, reqId) — (queue_slot_value, last_tried_input). The second argument there is not the request being "processed"; it's whichever ID was last compared before failing.

The two call sites disagree on which argument is "expected" and which is "processed", making off-chain monitoring and incident investigation harder. Document the parameter semantics in the error definition, or pick a single consistent convention.

[claude]

Silent skip + missing _dropQueuedRequest call creates queue/state divergence

_dropRequestsInternal only calls _dropQueuedRequest when request.status == RequestStatus.PENDING. If that guard is false, the request remains in _requestsQueue (the pointer-based queue is not updated) while the request's state is unchanged. The caller (startProcessingBatch) has already verified these IDs occupy specific queue slots via the validation loop — so a silent skip would leave a stale entry in the queue, stalling all subsequent dequeues.

In practice, any request in _requestsQueue should always be PENDING (only PENDING requests are enqueued; they exit via dequeue/drop). But given that the validation loop immediately before already confirmed the ID is in the queue, the guard mismatch should be an explicit revert rather than a silent skip, so bugs surface immediately rather than corrupting queue state silently.

[claude]

Rejection/cancellation path is still O(n) — batch rejection is O(k×n)

The PR description claims "both enqueue & dequeue operations are now O(1)", which is accurate for the happy path. However _dropQueuedRequest — called once per rejected request in _dropRequestsInternal and also on cancelRequest — is O(queue length) due to the element-shift loop. Processing a batch with k rejections against a queue of length n is therefore O(k×n).

For workloads where a large fraction of requests are rejected (e.g., spam bursts), this can exceed the original O(n) shift cost of the array-based approach. The docstring already acknowledges this ("O(n) operation"), but the PR summary should be updated to avoid overstating the improvement.

[claude]

Missing test: out-of-order arrays should revert

test_StartProcessingBatch_MultipleSuccessfulAndRejectRequests only tests the happy path where both arrays are in FIFO order. There is no test verifying that passing either array out of FIFO order reverts. For example:

// Queue: [req1, req2, req3, req4, req5]
// successfulRequestIds in reverse FIFO order → should revert
uint256[] memory successIds = new uint256[](2);
successIds[0] = req4;  // out of order
successIds[1] = req2;
uint256[] memory rejIds = new uint256[](3);
rejIds[0] = req1; rejIds[1] = req3; rejIds[2] = req5;
vm.expectRevert(...RequestProcessOutOfOrder...);
c.startProcessingBatch(successIds, rejIds);

Given the ordering constraint is the core correctness invariant of this PR, it warrants an explicit negative test.