diff --git a/src/abstract/AbstractRecipientRegistry.sol b/src/abstract/AbstractRecipientRegistry.sol index 66aa391..4c95e38 100644 --- a/src/abstract/AbstractRecipientRegistry.sol +++ b/src/abstract/AbstractRecipientRegistry.sol @@ -24,12 +24,6 @@ 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)) @@ -89,7 +83,8 @@ abstract contract AbstractRecipientRegistry is IRecipientRegistry, OwnableUpgrad // ============ Internal Functions ============ /// @notice Internal function to queue a recipient for addition - /// @param recipient Address to add to the queue + /// @param recipient Address to add to the queue; must be strictly greater than the last + /// queued address (ascending order) to maintain the sorted-array invariant /// @dev This is an internal function that should be called by derived contracts /// @dev Validates the recipient address and checks for duplicates before queuing /// @dev Emits RecipientQueued event with isAddition=true @@ -98,10 +93,13 @@ 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(); - $.isQueuedForAdditionMapping[recipient] = true; + uint256 len = $.queuedRecipientsForAddition.length; + if (len > 0 && uint160(recipient) <= uint160($.queuedRecipientsForAddition[len - 1])) { + revert QueueNotSorted(); + } + $.queuedRecipientsForAddition.push(recipient); emit RecipientQueued(recipient, true); } @@ -116,10 +114,13 @@ abstract contract AbstractRecipientRegistry is IRecipientRegistry, OwnableUpgrad 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(); - $.isQueuedForRemovalMapping[recipient] = true; + uint256 len = $.queuedRecipientsForRemoval.length; + if (len > 0 && uint160(recipient) <= uint160($.queuedRecipientsForRemoval[len - 1])) { + revert QueueNotSorted(); + } + $.queuedRecipientsForRemoval.push(recipient); emit RecipientQueued(recipient, false); } @@ -146,7 +147,6 @@ abstract contract AbstractRecipientRegistry is IRecipientRegistry, OwnableUpgrad address recipient = addedList[i]; $.recipients.push(recipient); $.isRecipientMapping[recipient] = true; - $.isQueuedForAdditionMapping[recipient] = false; emit RecipientAdded(recipient); } @@ -164,7 +164,6 @@ abstract contract AbstractRecipientRegistry is IRecipientRegistry, OwnableUpgrad if (recipient == removedList[j]) { shouldRemove = true; $.isRecipientMapping[recipient] = false; - $.isQueuedForRemovalMapping[recipient] = false; emit RecipientRemoved(recipient); break; } @@ -188,22 +187,14 @@ 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 { - AbstractRecipientRegistryStorage storage $ = _getAbstractRecipientRegistryStorage(); - for (uint256 i = 0; i < $.queuedRecipientsForAddition.length; i++) { - $.isQueuedForAdditionMapping[$.queuedRecipientsForAddition[i]] = false; - } - delete $.queuedRecipientsForAddition; + delete _getAbstractRecipientRegistryStorage().queuedRecipientsForAddition; } /// @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 { - AbstractRecipientRegistryStorage storage $ = _getAbstractRecipientRegistryStorage(); - for (uint256 i = 0; i < $.queuedRecipientsForRemoval.length; i++) { - $.isQueuedForRemovalMapping[$.queuedRecipientsForRemoval[i]] = false; - } - delete $.queuedRecipientsForRemoval; + delete _getAbstractRecipientRegistryStorage().queuedRecipientsForRemoval; } /// @notice Get all active recipients @@ -238,14 +229,36 @@ 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) { - return _getAbstractRecipientRegistryStorage().isQueuedForAdditionMapping[recipient]; + return _binarySearch(_getAbstractRecipientRegistryStorage().queuedRecipientsForAddition, 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) { - return _getAbstractRecipientRegistryStorage().isQueuedForRemovalMapping[recipient]; + return _binarySearch(_getAbstractRecipientRegistryStorage().queuedRecipientsForRemoval, recipient); + } + + /// @dev Binary search on a sorted address array + function _binarySearch(address[] storage arr, address target) internal view returns (bool) { + uint256 len = arr.length; + if (len == 0) return false; + + uint256 low = 0; + uint256 high = len; + uint256 targetUint = uint160(target); + + while (low < high) { + uint256 mid = low + (high - low) / 2; + uint256 midVal = uint160(arr[mid]); + if (midVal == targetUint) return true; + if (midVal < targetUint) { + low = mid + 1; + } else { + high = mid; + } + } + return false; } /// @notice Check if an address is currently an active recipient diff --git a/src/interfaces/IRecipientRegistry.sol b/src/interfaces/IRecipientRegistry.sol index 188e1bb..a139d82 100644 --- a/src/interfaces/IRecipientRegistry.sol +++ b/src/interfaces/IRecipientRegistry.sol @@ -40,6 +40,10 @@ interface IRecipientRegistry { /// @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 + /// @dev Queues must be submitted in ascending address order to enable O(1) enqueue + O(log n) lookup + error QueueNotSorted(); + /// @notice Thrown when attempting to queue a recipient when the queue has reached the maximum size error MaxQueueSizeReached(); diff --git a/test/AdminRecipientRegistry.t.sol b/test/AdminRecipientRegistry.t.sol index e73cc32..979517b 100644 --- a/test/AdminRecipientRegistry.t.sol +++ b/test/AdminRecipientRegistry.t.sol @@ -200,6 +200,113 @@ contract AdminRecipientRegistryTest is TestWrapper { registry.queueRecipientAddition(RECIPIENT_2); } + function test_WhenRe_queuingAfterRemoval() external { + // it should allow re-adding a previously removed recipient + vm.startPrank(ADMIN); + + registry.queueRecipientAddition(RECIPIENT_1); + registry.processQueue(); + assertTrue(registry.isRecipient(RECIPIENT_1)); + + registry.queueRecipientRemoval(RECIPIENT_1); + registry.processQueue(); + assertFalse(registry.isRecipient(RECIPIENT_1)); + + // Re-add should succeed (queue is empty, no stale state) + registry.queueRecipientAddition(RECIPIENT_1); + registry.processQueue(); + assertTrue(registry.isRecipient(RECIPIENT_1)); + + vm.stopPrank(); + } + + function test_WhenRe_queuingAfterClear() external { + // it should allow re-adding after clearing addition queue + vm.startPrank(ADMIN); + + registry.queueRecipientAddition(RECIPIENT_1); + assertTrue(registry.isQueuedForAddition(RECIPIENT_1)); + + registry.clearAdditionQueue(); + assertFalse(registry.isQueuedForAddition(RECIPIENT_1)); + + // Re-queue should succeed after clear + registry.queueRecipientAddition(RECIPIENT_1); + assertTrue(registry.isQueuedForAddition(RECIPIENT_1)); + registry.processQueue(); + assertTrue(registry.isRecipient(RECIPIENT_1)); + + vm.stopPrank(); + } + + function test_RevertWhen_QueuingDuplicateInSameBatch() external { + // it should revert with QueueNotSorted + vm.startPrank(ADMIN); + registry.queueRecipientAddition(RECIPIENT_1); + + vm.expectRevert(IRecipientRegistry.QueueNotSorted.selector); + registry.queueRecipientAddition(RECIPIENT_1); + vm.stopPrank(); + } + + function test_RevertWhen_QueuingOutOfOrder() external { + // it should revert with QueueNotSorted + vm.startPrank(ADMIN); + registry.queueRecipientAddition(RECIPIENT_2); + + // RECIPIENT_1 < RECIPIENT_2, so this should revert + vm.expectRevert(IRecipientRegistry.QueueNotSorted.selector); + registry.queueRecipientAddition(RECIPIENT_1); + vm.stopPrank(); + } + + function test_WhenCheckingCrossQueueIndependence() external { + // it should keep addition and removal queues independent + vm.startPrank(ADMIN); + + // Add recipients first so they can be queued for removal + registry.queueRecipientAddition(RECIPIENT_1); + registry.queueRecipientAddition(RECIPIENT_2); + registry.processQueue(); + + // Queue RECIPIENT_1 for removal and a new address for addition independently + registry.queueRecipientRemoval(RECIPIENT_1); + registry.queueRecipientAddition(RECIPIENT_3); + + assertTrue(registry.isQueuedForRemoval(RECIPIENT_1)); + assertTrue(registry.isQueuedForAddition(RECIPIENT_3)); + + registry.processQueue(); + + assertFalse(registry.isRecipient(RECIPIENT_1)); + assertTrue(registry.isRecipient(RECIPIENT_2)); + assertTrue(registry.isRecipient(RECIPIENT_3)); + + vm.stopPrank(); + } + + function test_WhenClearingRemovalQueueAndRe_queuing() external { + // it should allow re-queuing for removal after clearing + vm.startPrank(ADMIN); + + registry.queueRecipientAddition(RECIPIENT_1); + registry.processQueue(); + + registry.queueRecipientRemoval(RECIPIENT_1); + assertTrue(registry.isQueuedForRemoval(RECIPIENT_1)); + + registry.clearRemovalQueue(); + assertFalse(registry.isQueuedForRemoval(RECIPIENT_1)); + + // Re-queue for removal should succeed after clear + registry.queueRecipientRemoval(RECIPIENT_1); + assertTrue(registry.isQueuedForRemoval(RECIPIENT_1)); + registry.processQueue(); + assertFalse(registry.isRecipient(RECIPIENT_1)); + + vm.stopPrank(); + } + function test_WhenPerformingLargeScaleOperations() external { // it should handle 100 additions and 50 removals vm.startPrank(ADMIN); diff --git a/test/RecipientRegistry.t.sol b/test/RecipientRegistry.t.sol index a2ec0bc..963dc95 100644 --- a/test/RecipientRegistry.t.sol +++ b/test/RecipientRegistry.t.sol @@ -68,10 +68,10 @@ contract RecipientRegistryTest is TestWrapper { } function test_GivenTheRecipientIsAlreadyQueued() external { - // it should revert with RecipientAlreadyQueued + // it should revert with QueueNotSorted (duplicate = not sorted) registry.queueRecipientAddition(RECIPIENT_1); - vm.expectRevert(IRecipientRegistry.RecipientAlreadyQueued.selector); + vm.expectRevert(IRecipientRegistry.QueueNotSorted.selector); registry.queueRecipientAddition(RECIPIENT_1); } @@ -102,13 +102,13 @@ contract RecipientRegistryTest is TestWrapper { } function test_GivenTheRecipientIsAlreadyQueuedForRemoval() external { - // it should revert with RecipientAlreadyQueued + // it should revert with QueueNotSorted (duplicate = not sorted) registry.queueRecipientAddition(RECIPIENT_1); registry.processQueue(); registry.queueRecipientRemoval(RECIPIENT_1); - vm.expectRevert(IRecipientRegistry.RecipientAlreadyQueued.selector); + vm.expectRevert(IRecipientRegistry.QueueNotSorted.selector); registry.queueRecipientRemoval(RECIPIENT_1); }