feat: switch to mapping instead of linear search by exo404 · Pull Request #90 · BreadchainCoop/crowdstake.fun

exo404 · 2026-03-17T18:34:38Z

This solves #45

RonTuretzky · 2026-03-17T19:49:25Z


+    /// @notice Mapping to quickly check if an address is queued recipient
+    /// @dev Maps recipient address to true if queued, false otherwise
+    mapping(address => bool) public isQueued;


dont we need to clear this queue after they're added?

@exo404 and we should be handling removals as well?

Copilot

Pull request overview

This PR aims to optimize recipient queue duplicate detection in AbstractRecipientRegistry by switching the addition-queue duplicate check from an O(n) linear scan to an O(1) mapping lookup, addressing performance concerns raised in #45.

Changes:

Added an isQueued mapping to track whether an address is queued.
Updated _queueForAddition to use the mapping instead of iterating queuedRecipientsForAddition.

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

Copilot · 2026-03-19T20:59:45Z

        if (isRecipientMapping[recipient]) revert RecipientAlreadyExists();
+        if (isQueued[recipient]) revert RecipientAlreadyQueued();

-        // Check if already queued to prevent duplicates
-        for (uint256 i = 0; i < queuedRecipientsForAddition.length; i++) {
-            if (queuedRecipientsForAddition[i] == recipient) {
-                revert RecipientAlreadyQueued();
-            }
-        }
-
+        isQueued[recipient] = true;
        queuedRecipientsForAddition.push(recipient);
        emit RecipientQueued(recipient, true);


isQueued is set to true when queueing an addition, but it is never cleared when the addition queue is processed (_processQueue) or manually cleared (clearAdditionQueue). This makes it impossible to re-queue the same recipient later (e.g., add -> remove -> attempt to add again will revert with RecipientAlreadyQueued). Ensure the mapping is reset when entries leave the addition queue (during processing and when clearing).

Copilot · 2026-03-19T20:59:46Z

    function _queueForAddition(address recipient) internal {
        if (recipient == address(0)) revert InvalidRecipient();
        if (isRecipientMapping[recipient]) revert RecipientAlreadyExists();
+        if (isQueued[recipient]) revert RecipientAlreadyQueued();

-        // Check if already queued to prevent duplicates
-        for (uint256 i = 0; i < queuedRecipientsForAddition.length; i++) {
-            if (queuedRecipientsForAddition[i] == recipient) {
-                revert RecipientAlreadyQueued();
-            }
-        }
-
+        isQueued[recipient] = true;
        queuedRecipientsForAddition.push(recipient);
        emit RecipientQueued(recipient, true);


PR description references solving #45 (duplicate checks for both addition and removal queues), but this change only adds an O(1) check for the addition queue. The removal queue still uses a linear scan, so large removal queues will keep the same O(n) behavior. Either extend the mapping approach to removals as well, or adjust the PR/issue linkage to reflect the remaining work.

Copilot · 2026-03-19T20:59:46Z

        if (isRecipientMapping[recipient]) revert RecipientAlreadyExists();
+        if (isQueued[recipient]) revert RecipientAlreadyQueued();

-        // Check if already queued to prevent duplicates
-        for (uint256 i = 0; i < queuedRecipientsForAddition.length; i++) {
-            if (queuedRecipientsForAddition[i] == recipient) {
-                revert RecipientAlreadyQueued();
-            }
-        }
-
+        isQueued[recipient] = true;
        queuedRecipientsForAddition.push(recipient);
        emit RecipientQueued(recipient, true);


This introduces new queue-tracking behavior via isQueued, but there are existing Foundry tests for registries and none currently cover re-adding a recipient after it was previously added then removed, or re-queuing after clearAdditionQueue(). Adding tests for these scenarios would catch regressions around clearing the queued mapping when entries leave the queue.

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

exo404 · 2026-03-31T12:13:52Z

@RonTuretzky fixed and solved conflicts with main branch to align to the EIP 7201 standard

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

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

Copilot · 2026-03-31T17:13:43Z

+        AbstractRecipientRegistryStorage storage $ = _getAbstractRecipientRegistryStorage();
+        for (uint256 i = 0; i < $.queuedRecipientsForAddition.length; i++) {
+            $.isQueuedForAdditionMapping[$.queuedRecipientsForAddition[i]] = false;
+        }
+        delete $.queuedRecipientsForAddition;


Clearing the addition queue is now O(n) and can become uncallable if the queue grows large enough to exceed the block gas limit, leaving the owner unable to cancel pending additions. Consider a constant-gas invalidation pattern (e.g., a queue “epoch/nonce” plus mapping to the epoch) or a batched clearAdditionQueue(uint256 start, uint256 count) approach so the queue can always be cleared progressively.

Copilot · 2026-03-31T17:13:44Z

+        AbstractRecipientRegistryStorage storage $ = _getAbstractRecipientRegistryStorage();
+        for (uint256 i = 0; i < $.queuedRecipientsForRemoval.length; i++) {
+            $.isQueuedForRemovalMapping[$.queuedRecipientsForRemoval[i]] = false;
+        }
+        delete $.queuedRecipientsForRemoval;


Same concern as clearAdditionQueue: this O(n) loop can make clearRemovalQueue fail for large queues due to gas limits. A batched clear function or an epoch-based invalidation mechanism would avoid a potential operational dead-end where the removal queue cannot be cleared.

@RonTuretzky wdyt? This is called by owner only

Best practice would be to limited to some sort of upper limit with like a constant

Tranquil-Flow

Overall

The mapping-based O(1) duplicate detection is the right approach and the ERC-7201 namespaced storage usage is correct. However, two blocking issues need resolution before merge.

🔴 No test coverage

The PR modifies only AbstractRecipientRegistry.sol — zero test files are included. The following scenarios need coverage:

Re-queue after removal: Add → process → remove → process → re-add. If the mapping isn't cleared properly, re-adding is permanently blocked.
Re-queue after clear: Queue → clearAdditionQueue() → re-queue. Verifies the mapping reset in clear functions works.
Double-queue revert: Confirm RecipientAlreadyQueued fires when the same address is queued twice.
Cross-queue independence: An address queued for addition should still be queueable for removal independently.

🔴 `clearAdditionQueue` and `clearRemovalQueue` are now O(n) and gas-unbounded

The original implementations were delete $.queuedRecipientsForAddition — a single SSTORE. Now they iterate the full array to clear each mapping entry. If the queue grows large enough, these owner-only functions will exceed the block gas limit and become permanently uncallable.

Two options:

Cap queue length with a constant (e.g., MAX_QUEUE_SIZE = 100) enforced in _queueForAddition / _queueForRemoval.
Epoch-based invalidation: Increment a queueEpoch counter; key becomes keccak256(abi.encode(recipient, epoch)) so clearing is O(1).

🟡 New revert path in `_queueForRemoval`

The address(0) check is a reasonable hardening, but it's a behavioral change — previously address(0) would fail on !$.isRecipientMapping[recipient] with a different revert. Worth noting in the PR description.

✅ What's working well

Correct use of ERC-7201 namespaced storage — avoids the layout collision flagged on the earlier approach
Separate mappings for add vs. remove queues — handles the edge case of an address being in both
Mapping cleanup on all exit paths (_processQueue, clearAdditionQueue, clearRemovalQueue)
Good iteration through review feedback

RonTuretzky · 2026-04-15T23:40:44Z

Superseded by #118, which cherry-picks these commits onto the current main with all review feedback addressed (tests, MAX_QUEUE_SIZE cap, both queues). The sorted-array rewrite in #126 further improves the approach.

exo404 requested a review from RonTuretzky March 17, 2026 18:34

exo404 self-assigned this Mar 17, 2026

exo404 added the enhancement New feature or request label Mar 17, 2026

exo404 linked an issue Mar 17, 2026 that may be closed by this pull request

Optimize Recipient Queue Lookup in BaseRecipeIntRegistry for Large Queues (addition and removal) #45

Closed

RonTuretzky requested changes Mar 17, 2026

View reviewed changes

RonTuretzky requested review from Copilot March 19, 2026 20:55

Copilot started reviewing on behalf of RonTuretzky March 19, 2026 20:56 View session

Copilot AI reviewed Mar 19, 2026

View reviewed changes

feat: switch to mapping instead of linear search-

4d95fb9

exo404 force-pushed the 45-optimize-recipient-queue-lookup-in-baserecipeintregistry-for-large-queues-addition-and-removal branch from 6664e00 to 4d95fb9 Compare March 31, 2026 11:49

RonTuretzky requested a review from Copilot March 31, 2026 17:11

Copilot AI reviewed Mar 31, 2026

View reviewed changes

RonTuretzky requested changes Mar 31, 2026

View reviewed changes

Comment thread src/abstract/AbstractRecipientRegistry.sol

fix: add natspec to recipient mapping

f9080c1

Tranquil-Flow reviewed Apr 4, 2026

View reviewed changes

exo404 added 2 commits April 7, 2026 15:20

fix: add a maximum queue size constant to gas bound functions

316d964

test: update tests

76da7a8

RonTuretzky mentioned this pull request Apr 7, 2026

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

Open

4 tasks

RonTuretzky mentioned this pull request Apr 15, 2026

fix: optimize queue lookup with mapping-based O(1) duplicate detection (#45) #118

Merged

6 tasks

RonTuretzky closed this Apr 15, 2026

Conversation

exo404 commented Mar 17, 2026

Uh oh!

RonTuretzky Mar 17, 2026

Choose a reason for hiding this comment

Uh oh!

RonTuretzky Mar 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI Mar 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 19, 2026

Choose a reason for hiding this comment

Uh oh!

exo404 Mar 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 19, 2026

Choose a reason for hiding this comment

Uh oh!

exo404 Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

exo404 commented Mar 31, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

exo404 Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

RonTuretzky Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

exo404 Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Tranquil-Flow left a comment

Choose a reason for hiding this comment

Overall

🔴 No test coverage

🔴 clearAdditionQueue and clearRemovalQueue are now O(n) and gas-unbounded

🟡 New revert path in _queueForRemoval

✅ What's working well

Uh oh!

RonTuretzky commented Apr 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

exo404 Mar 31, 2026 •

edited

Loading

🔴 `clearAdditionQueue` and `clearRemovalQueue` are now O(n) and gas-unbounded

🟡 New revert path in `_queueForRemoval`