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

Show file tree
Hide file tree
Changes from all 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
62 changes: 31 additions & 31 deletions src/abstract/AbstractRecipientRegistry.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,15 @@ 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))
/// keccak256(abi.encode(uint256(keccak256("crowdstake.storage.AbstractRecipientRegistry")) - 1)) & ~bytes32(uint256(0xff))
bytes32 private constant ABSTRACT_RECIPIENT_REGISTRY_STORAGE =
0x347caeef91698b68f09c13de18e96db5bda028445fd11b86dc029946f360f200;

Expand All @@ -36,6 +42,9 @@ abstract contract AbstractRecipientRegistry is IRecipientRegistry, OwnableUpgrad
}
}

/// @notice Maximum queue size
uint256 private constant MAX_QUEUE_SIZE = 100;

// ============ Public Getters ============

/// @notice Array of active recipient addresses
Expand Down Expand Up @@ -89,14 +98,10 @@ 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();
if ($.queuedRecipientsForAddition.length + 1 > MAX_QUEUE_SIZE) revert MaxQueueSizeReached();

// 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);
}
Expand All @@ -109,15 +114,12 @@ 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();
if ($.queuedRecipientsForRemoval.length + 1 > MAX_QUEUE_SIZE) revert MaxQueueSizeReached();

// 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 +146,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 +164,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 +188,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 +238,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
3 changes: 3 additions & 0 deletions src/interfaces/IRecipientRegistry.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,9 @@ interface IRecipientRegistry {
/// @notice Thrown when attempting to queue a recipient that is already queued
error RecipientAlreadyQueued();

/// @notice Thrown when attempting to queue a recipient when the queue reached the maximum size
error MaxQueueSizeReached();

/// @notice Queue a recipient for addition to the registry
/// @dev Access control varies by implementation (admin-only vs recipient voting)
/// @dev The recipient will be added when the queue is processed
Expand Down
63 changes: 61 additions & 2 deletions test/RecipientRegistry.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,46 @@ contract RecipientRegistryTest is TestWrapper {
assertEq(registry.getQueuedAdditions().length, 0);
}

function test_ReQueueRecipientAfterRemoval() public {
registry.queueRecipientAddition(RECIPIENT_1);
registry.processQueue();

registry.queueRecipientRemoval(RECIPIENT_1);
registry.processQueue();

assertFalse(registry.isRecipient(RECIPIENT_1));
assertFalse(registry.isQueuedForRemoval(RECIPIENT_1));

registry.queueRecipientAddition(RECIPIENT_1);

assertTrue(registry.isQueuedForAddition(RECIPIENT_1));
assertEq(registry.getQueuedAdditions().length, 1);

registry.processQueue();

assertTrue(registry.isRecipient(RECIPIENT_1));
assertFalse(registry.isQueuedForAddition(RECIPIENT_1));
assertEq(registry.getRecipientCount(), 1);
}

function test_ReQueueRecipientAfterClearingAdditionQueue() public {
registry.queueRecipientAddition(RECIPIENT_1);

registry.clearAdditionQueue();

assertEq(registry.getQueuedAdditions().length, 0);
assertFalse(registry.isQueuedForAddition(RECIPIENT_1));

registry.queueRecipientAddition(RECIPIENT_1);
assertTrue(registry.isQueuedForAddition(RECIPIENT_1));

registry.processQueue();

assertTrue(registry.isRecipient(RECIPIENT_1));
assertFalse(registry.isQueuedForAddition(RECIPIENT_1));
assertEq(registry.getRecipientCount(), 1);
}

function test_ClearRemovalQueue() public {
// Add recipients first
registry.queueRecipientAddition(RECIPIENT_1);
Expand Down Expand Up @@ -221,7 +261,7 @@ contract RecipientRegistryTest is TestWrapper {
function test_RevertOnDuplicateQueuedAddition() public {
registry.queueRecipientAddition(RECIPIENT_1);

vm.expectRevert();
vm.expectRevert(IRecipientRegistry.RecipientAlreadyQueued.selector);
registry.queueRecipientAddition(RECIPIENT_1);
}

Expand All @@ -236,10 +276,29 @@ contract RecipientRegistryTest is TestWrapper {

registry.queueRecipientRemoval(RECIPIENT_1);

vm.expectRevert();
vm.expectRevert(IRecipientRegistry.RecipientAlreadyQueued.selector);
registry.queueRecipientRemoval(RECIPIENT_1);
}

function test_AdditionAndRemovalQueuesAreIndependent() public {
registry.queueRecipientAddition(RECIPIENT_1);
registry.processQueue();

registry.queueRecipientAddition(RECIPIENT_2);
registry.queueRecipientRemoval(RECIPIENT_1);

assertTrue(registry.isQueuedForAddition(RECIPIENT_2));
assertTrue(registry.isQueuedForRemoval(RECIPIENT_1));
assertFalse(registry.isQueuedForAddition(RECIPIENT_1));
assertFalse(registry.isQueuedForRemoval(RECIPIENT_2));

registry.processQueue();

assertFalse(registry.isRecipient(RECIPIENT_1));
assertTrue(registry.isRecipient(RECIPIENT_2));
assertEq(registry.getRecipientCount(), 1);
}

function test_OnlyOwnerCanQueue() public {
vm.prank(address(0xdead));
vm.expectRevert();
Expand Down
Loading