Skip to content

feat: switch to mapping instead of linear search #90

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
exo404 wants to merge 4 commits into from
Closed

feat: switch to mapping instead of linear search #90

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4d95fb9
feat: switch to mapping instead of linear search-
exo404 Mar 17, 2026
f9080c1
fix: add natspec to recipient mapping
exo404 Mar 31, 2026
316d964
fix: add a maximum queue size constant to gas bound functions
exo404 Apr 7, 2026
76da7a8
test: update tests
exo404 Apr 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 25 additions & 30 deletions src/abstract/AbstractRecipientRegistry.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,12 @@ abstract contract AbstractRecipientRegistry is IRecipientRegistry, OwnableUpgrad
/// @notice Mapping to quickly check if an address is an active recipient
/// @dev Maps recipient address to true if active, false otherwise
mapping(address => bool) isRecipientMapping;
/// @notice Mapping to quickly check if an address is queued for addition
/// @dev Maps recipient address to true if queued for addition, false otherwise
mapping(address => bool) isQueuedForAdditionMapping;
/// @notice Mapping to quickly check if an address is queued for removal
/// @dev Maps recipient address to true if queued for removal, false otherwise
mapping(address => bool) isQueuedForRemovalMapping;
}

// keccak256(abi.encode(uint256(keccak256("crowdstake.storage.AbstractRecipientRegistry")) - 1)) & ~bytes32(uint256(0xff))
Expand Down Expand Up @@ -89,14 +95,9 @@ abstract contract AbstractRecipientRegistry is IRecipientRegistry, OwnableUpgrad
AbstractRecipientRegistryStorage storage $ = _getAbstractRecipientRegistryStorage();
if (recipient == address(0)) revert InvalidRecipient();
if ($.isRecipientMapping[recipient]) revert RecipientAlreadyExists();
if ($.isQueuedForAdditionMapping[recipient]) revert RecipientAlreadyQueued();

// Check if already queued to prevent duplicates
for (uint256 i = 0; i < $.queuedRecipientsForAddition.length; i++) {
if ($.queuedRecipientsForAddition[i] == recipient) {
revert RecipientAlreadyQueued();
}
}

$.isQueuedForAdditionMapping[recipient] = true;
$.queuedRecipientsForAddition.push(recipient);
emit RecipientQueued(recipient, true);
Comment on lines 97 to 106
Copy link

Copilot AI Mar 19, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

@exo404 exo404 Mar 31, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

b39b223

}
Expand All @@ -109,15 +110,11 @@ abstract contract AbstractRecipientRegistry is IRecipientRegistry, OwnableUpgrad
/// @dev Access control should be implemented in the calling public function
function _queueForRemoval(address recipient) internal {
AbstractRecipientRegistryStorage storage $ = _getAbstractRecipientRegistryStorage();
if (recipient == address(0)) revert InvalidRecipient();
if (!$.isRecipientMapping[recipient]) revert RecipientNotFound();
if ($.isQueuedForRemovalMapping[recipient]) revert RecipientAlreadyQueued();

// Check if already queued for removal to prevent duplicates
for (uint256 i = 0; i < $.queuedRecipientsForRemoval.length; i++) {
if ($.queuedRecipientsForRemoval[i] == recipient) {
revert RecipientAlreadyQueued();
}
}

$.isQueuedForRemovalMapping[recipient] = true;
$.queuedRecipientsForRemoval.push(recipient);
emit RecipientQueued(recipient, false);
}
Expand All @@ -144,6 +141,7 @@ abstract contract AbstractRecipientRegistry is IRecipientRegistry, OwnableUpgrad
address recipient = addedList[i];
$.recipients.push(recipient);
$.isRecipientMapping[recipient] = true;
$.isQueuedForAdditionMapping[recipient] = false;
emit RecipientAdded(recipient);
}

Expand All @@ -161,6 +159,7 @@ abstract contract AbstractRecipientRegistry is IRecipientRegistry, OwnableUpgrad
if (recipient == removedList[j]) {
shouldRemove = true;
$.isRecipientMapping[recipient] = false;
$.isQueuedForRemovalMapping[recipient] = false;
emit RecipientRemoved(recipient);
break;
}
Expand All @@ -184,14 +183,22 @@ abstract contract AbstractRecipientRegistry is IRecipientRegistry, OwnableUpgrad
/// @dev Only owner can clear the queue. Use this to cancel all pending additions
/// @dev This will remove all addresses from the addition queue without adding them
function clearAdditionQueue() external onlyOwner {
delete _getAbstractRecipientRegistryStorage().queuedRecipientsForAddition;
AbstractRecipientRegistryStorage storage $ = _getAbstractRecipientRegistryStorage();
for (uint256 i = 0; i < $.queuedRecipientsForAddition.length; i++) {
$.isQueuedForAdditionMapping[$.queuedRecipientsForAddition[i]] = false;
}
delete $.queuedRecipientsForAddition;
Comment on lines +191 to +195
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
}

/// @notice Clear the removal queue without processing
/// @dev Only owner can clear the queue. Use this to cancel all pending removals
/// @dev This will remove all addresses from the removal queue without removing them
function clearRemovalQueue() external onlyOwner {
delete _getAbstractRecipientRegistryStorage().queuedRecipientsForRemoval;
AbstractRecipientRegistryStorage storage $ = _getAbstractRecipientRegistryStorage();
for (uint256 i = 0; i < $.queuedRecipientsForRemoval.length; i++) {
$.isQueuedForRemovalMapping[$.queuedRecipientsForRemoval[i]] = false;
}
delete $.queuedRecipientsForRemoval;
Comment on lines +202 to +206
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

@exo404 exo404 Mar 31, 2026

Choose a reason for hiding this comment

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

@RonTuretzky wdyt? This is called by owner only

Copy link
Copy Markdown
Contributor

@RonTuretzky RonTuretzky Mar 31, 2026

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

@exo404 exo404 Apr 7, 2026

Choose a reason for hiding this comment

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

316d964

}

/// @notice Get all active recipients
Expand Down Expand Up @@ -226,26 +233,14 @@ abstract contract AbstractRecipientRegistry is IRecipientRegistry, OwnableUpgrad
/// @param recipient Address to check in the addition queue
/// @return isQueued True if the address is queued for addition, false otherwise
function isQueuedForAddition(address recipient) external view returns (bool isQueued) {
AbstractRecipientRegistryStorage storage $ = _getAbstractRecipientRegistryStorage();
for (uint256 i = 0; i < $.queuedRecipientsForAddition.length; i++) {
if ($.queuedRecipientsForAddition[i] == recipient) {
return true;
}
}
return false;
return _getAbstractRecipientRegistryStorage().isQueuedForAdditionMapping[recipient];
}

/// @notice Check if an address is queued for removal
/// @param recipient Address to check in the removal queue
/// @return isQueued True if the address is queued for removal, false otherwise
function isQueuedForRemoval(address recipient) external view returns (bool isQueued) {
AbstractRecipientRegistryStorage storage $ = _getAbstractRecipientRegistryStorage();
for (uint256 i = 0; i < $.queuedRecipientsForRemoval.length; i++) {
if ($.queuedRecipientsForRemoval[i] == recipient) {
return true;
}
}
return false;
return _getAbstractRecipientRegistryStorage().isQueuedForRemovalMapping[recipient];
}

/// @notice Check if an address is currently an active recipient
Expand Down
Loading