Skip to content

refactor: replace queue mappings with sorted-array invariant (#116)#126

Open
RonTuretzky wants to merge 2 commits into
RonTuretzky/107-twvp-scalingfrom
RonTuretzky/116-sorted-queue
Open

refactor: replace queue mappings with sorted-array invariant (#116)#126
RonTuretzky wants to merge 2 commits into
RonTuretzky/107-twvp-scalingfrom
RonTuretzky/116-sorted-queue

Conversation

@RonTuretzky
Copy link
Copy Markdown
Contributor

Summary

  • Replace mapping-based O(1) duplicate detection with sorted-array invariant + O(log n) binary search
  • Require ascending address order for queue submissions (QueueNotSorted error)
  • Simplify clearAdditionQueue/clearRemovalQueue back to simple delete
  • Keep MAX_QUEUE_SIZE = 100 cap from PR 1
  • Add 6 new tests for sorted order, re-queue, and cross-queue independence

Revisits #45 (structural improvement over the mapping approach in PR 1)
Supersedes #116

Stack: PR 9 of 10 (0.0.2) — stacked on #125

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors recipient queue duplicate detection away from per-queue mappings into a sorted-address invariant, enabling cheaper queue clearing and O(log n) queued-membership checks via binary search across the queued arrays.

Changes:

  • Introduces QueueNotSorted error and requires strictly ascending address order for queue submissions.
  • Removes queue-membership mappings from storage usage; clearAdditionQueue/clearRemovalQueue revert to delete.
  • Updates/extends tests to assert the new revert behavior and to cover re-queueing and cross-queue behavior.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
src/abstract/AbstractRecipientRegistry.sol Enforces sorted-queue invariant, simplifies queue clearing, and implements binary search for isQueuedFor*.
src/interfaces/IRecipientRegistry.sol Adds QueueNotSorted error and documents the new ordering requirement.
test/RecipientRegistry.t.sol Updates expected revert selector for duplicate queueing to QueueNotSorted.
test/AdminRecipientRegistry.t.sol Adds new tests for sorted order enforcement, re-queue after removal/clear, and cross-queue independence.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/interfaces/IRecipientRegistry.sol Outdated
error RecipientAlreadyQueued();

/// @notice Thrown when a recipient address is not greater than the last queued address
/// @dev Queues must be submitted in ascending address order to enable O(log n) duplicate detection
Copy link

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The QueueNotSorted NatSpec is inaccurate: the ascending-order invariant enables O(1) duplicate prevention at enqueue time (by comparing to the last element), while O(log n) applies to the isQueuedFor* lookup via binary search. Please update the comment to reflect the actual complexity/behavior so integrators don’t misunderstand the tradeoff.

Suggested change
/// @dev Queues must be submitted in ascending address order to enable O(log n) duplicate detection
/// @dev Queues must be submitted in strictly ascending address order so enqueue-time
/// @dev duplicate/order validation can be done in O(1) by comparing against the last
/// @dev queued element; the sorted invariant also enables O(log n) `isQueuedFor*`
/// @dev lookups via binary search.

Copilot uses AI. Check for mistakes.
Comment on lines 40 to +43
/// @notice Thrown when attempting to queue a recipient that is already queued
error RecipientAlreadyQueued();

/// @notice Thrown when a recipient address is not greater than the last queued address
Copy link

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RecipientAlreadyQueued remains in the interface, but no implementation appears to revert with it anymore (duplicate and out-of-order both revert QueueNotSorted). Either (a) reintroduce RecipientAlreadyQueued for the recipient == lastQueued case, or (b) explicitly deprecate/remove RecipientAlreadyQueued to avoid exposing an error that will never be thrown.

Suggested change
/// @notice Thrown when attempting to queue a recipient that is already queued
error RecipientAlreadyQueued();
/// @notice Thrown when a recipient address is not greater than the last queued address
/// @notice Thrown when a queued recipient address is not strictly greater than the last queued address
/// @dev Covers both duplicate recipients and out-of-order queue submissions

Copilot uses AI. Check for mistakes.
Comment on lines +97 to +100
uint256 len = $.queuedRecipientsForAddition.length;
if (len > 0 && uint160(recipient) <= uint160($.queuedRecipientsForAddition[len - 1])) {
revert QueueNotSorted();
}
Copy link

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The enqueue path now enforces a strict ascending-address invariant (reverting QueueNotSorted when recipient <= lastQueued). Consider documenting this requirement in the function’s NatSpec (either here or on the external queueing functions) since it’s a behavioral/API constraint callers must follow.

Copilot uses AI. Check for mistakes.
@RonTuretzky RonTuretzky force-pushed the RonTuretzky/107-twvp-scaling branch from 55b1a25 to 69057ed Compare April 16, 2026 01:29
Replace O(1) mapping-based duplicate detection with sorted-array
invariant + binary search for O(log n) lookup. Require ascending
address order for queue submissions. Simplify clear operations back
to simple delete. Add QueueNotSorted error. Add 6 new tests for
sorted order enforcement and re-queue scenarios.
@RonTuretzky RonTuretzky force-pushed the RonTuretzky/116-sorted-queue branch from fb31c91 to eae8aac Compare April 16, 2026 01:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants