DEVSVCS-2328: remove duplicate request digests

contracts/src/v0.8/workflow/dev/v2/WorkflowRegistry.sol

@@ -89,10 +89,14 @@ contract WorkflowRegistry is Ownable2StepMsgSender, ITypeAndVersion {
   /// When a workflow is paused it is removed from the don family.
   mapping(bytes32 rid => bytes32 donHash) private s_donByWorkflowRid;
 
-  /// @dev Tracking allowlisted requests for the owner address, required to enable anyone to verify off-chain requests.
-  mapping(bytes32 ownerDigestHash => uint32 expiryTimestamp) private s_requestsAllowlist;
-  /// @dev Storing allowlisted requests for all owners, enabling fetching all non-expired requests
-  OwnerAllowlistedRequest[] private s_requestAllowlistArray;
+  /// @dev Fast lookup for allowlisted requests. Key is hash of owner + request digest, value is
+  /// index in the s_allowlistedRequestsData array pushed by one. Pushing index by one avoids collisions between an
+  /// entry at the zero index and entry that doesn't exist.
+  /// This is used for tracking allowlisted requests for the owner address, required to enable anyone to verify
+  /// off-chain requests.
+  mapping(bytes32 => uint256) private s_allowlistedRequestsIndexMap;
+  /// @dev Array storing all allowlisted request data for enumeration and pagination.
+  OwnerAllowlistedRequest[] private s_allowlistedRequestsData;
   /// @dev Map each owner address to their arbitrary config. Can be used to control billing parameters or any other data
   /// per owner
   mapping(address owner => bytes config) private s_ownerConfig;
@@ -204,7 +208,8 @@ contract WorkflowRegistry is Ownable2StepMsgSender, ITypeAndVersion {
     uint8 maxTagLen; // Cap for `tag` (0 ➜ unlimited)
     uint8 maxUrlLen; // Cap for each URL (0 ➜ unlimited)
     uint16 maxAttrLen; // Cap for `attributes` (0 ➜ unlimited)
-    uint32 maxExpiryLen; // Cap for every allowlisted request expiration timestamp (0 ➜ unlimited)
+    uint32 maxExpiryLen; // Maximum window in seconds from now (0 ⇒ never expires) for every allowlisted request
+      // expiration timestamp.
   }
 
   /// @dev Struct for WorkflowMetadata. This is used to store the workflow metadata.
@@ -1299,10 +1304,23 @@ contract WorkflowRegistry is Ownable2StepMsgSender, ITypeAndVersion {
     }
     if (!s_linkedOwners.contains(msg.sender)) revert OwnershipLinkDoesNotExist(msg.sender);
 
-    s_requestsAllowlist[keccak256(abi.encode(msg.sender, requestDigest))] = expiryTimestamp;
-    s_requestAllowlistArray.push(
-      OwnerAllowlistedRequest({owner: msg.sender, requestDigest: requestDigest, expiryTimestamp: expiryTimestamp})
-    );
+    bytes32 key = keccak256(abi.encode(msg.sender, requestDigest));
+
+    // non-zero index means that the request digest already exists
+    uint256 storedIndex = s_allowlistedRequestsIndexMap[key];
+
+    if (storedIndex != 0) {
+      // index is pushed by one when stored, so we need to subtract one to get the correct index
+      // then update existing request digest with a new expiry timestamp
+      s_allowlistedRequestsData[storedIndex - 1].expiryTimestamp = expiryTimestamp;
+    } else {
+      // push index by one to avoid collisions between an entry at the zero index and entry
+      // that doesn't exist in the mapping
+      s_allowlistedRequestsIndexMap[key] = s_allowlistedRequestsData.length + 1;
+      s_allowlistedRequestsData.push(
+        OwnerAllowlistedRequest({owner: msg.sender, requestDigest: requestDigest, expiryTimestamp: expiryTimestamp})
+      );
+    }
     emit RequestAllowlisted(msg.sender, requestDigest, expiryTimestamp);
   }
 
@@ -1313,22 +1331,36 @@ contract WorkflowRegistry is Ownable2StepMsgSender, ITypeAndVersion {
   /// @param requestDigest    Unique identifier for the request (hash of the request payload).
   /// @return bool            True if the request is allowlisted and not expired, false otherwise.
   function isRequestAllowlisted(address owner, bytes32 requestDigest) external view returns (bool) {
-    return s_requestsAllowlist[keccak256(abi.encode(owner, requestDigest))] > block.timestamp;
-  }
-
+    bytes32 key = keccak256(abi.encode(owner, requestDigest));
+    uint256 storedIndex = s_allowlistedRequestsIndexMap[key];
+    if (storedIndex == 0) return false; // zero index indicates that request is not found
+    // index is pushed by one when stored, so we need to subtract one to get the correct index
+    OwnerAllowlistedRequest storage request = s_allowlistedRequestsData[storedIndex - 1];
+    return request.expiryTimestamp > block.timestamp;
+  }
+
+  /// @notice     Returns a paginated list of allowlisted requests across all owners.
+  /// @dev        - Reads a slice of the allowlisted requests starting at `start` and spanning up to `limit` elements.
+  ///             - Expired entries (where `expiryTimestamp <= block.timestamp`) are filtered out.
+  ///             - The returned array may therefore be shorter than `limit`.
+  ///             - Does not revert on out-of-range pagination: if `start >= total`, returns an empty array.
+  /// @param start  Zero-based index into the allowlist at which to begin.
+  /// @param limit  Maximum number of entries to return from `start`.
+  /// @return allowlistedRequests  Array of {requestDigest, owner, expiryTimestamp} structs
+  ///                              for all non-expired requests found in the page slice.
   function getAllowlistedRequests(
     uint256 start,
     uint256 limit
   ) external view returns (OwnerAllowlistedRequest[] memory allowlistedRequests) {
-    uint256 total = s_requestAllowlistArray.length;
+    uint256 total = s_allowlistedRequestsData.length;
     uint256 pageCount = _getPageCount(total, start, limit);
 
     if (pageCount == 0) return new OwnerAllowlistedRequest[](0);
 
     allowlistedRequests = new OwnerAllowlistedRequest[](pageCount);
     uint256 addedCount = 0;
     for (uint256 i = 0; i < pageCount; ++i) {
-      OwnerAllowlistedRequest storage request = s_requestAllowlistArray[start + i];
+      OwnerAllowlistedRequest storage request = s_allowlistedRequestsData[start + i];
       if (request.expiryTimestamp > block.timestamp) {
         allowlistedRequests[addedCount] = request;
         ++addedCount;
@@ -1346,8 +1378,12 @@ contract WorkflowRegistry is Ownable2StepMsgSender, ITypeAndVersion {
     return allowlistedRequests;
   }
 
+  /// @notice Returns the total number of allowlisted requests across all owners.
+  /// @dev    Use this in tandem with `getAllowlistedRequests(start, limit)` to
+  ///         page through the allowlisted requests.
+  /// @return The total count of allowlisted requests stored.
   function totalAllowlistedRequests() external view returns (uint256) {
-    return s_requestAllowlistArray.length;
+    return s_allowlistedRequestsData.length;
   }
 
   // ================================================================

contracts/src/v0.8/workflow/dev/v2/test/WorkflowRegistry/WorkflowRegistry.allowlistRequest.t.sol

@@ -45,8 +45,91 @@ contract WorkflowRegistry_allowlistRequest is WorkflowRegistrySetup {
     s_registry.allowlistRequest(requestDigest, expiryTimestamp);
   }
 
-  function test_allowlistRequest_WhenTheUserIsLinked() external {
-    //it should allowlist the request digest
+  // When the user is linked
+  function test_allowlistRequest_WhenTheUserAlreadyHasARequest() external {
+    // It should update the existing request in-place without growing the array
+    bytes32 requestDigest = keccak256("duplicate-test-request");
+    uint32 initialExpiry = uint32(block.timestamp + 1 hours);
+    uint32 updatedExpiry = uint32(block.timestamp + 2 hours);
+
+    // Link the owner first
+    _linkOwner(s_user);
+
+    // Initial allowlist
+    vm.expectEmit(true, true, true, false);
+    emit WorkflowRegistry.RequestAllowlisted(s_user, requestDigest, initialExpiry);
+    vm.prank(s_user);
+    s_registry.allowlistRequest(requestDigest, initialExpiry);
+
+    // Verify the request is allowlisted with initial expiry
+    assertTrue(s_registry.isRequestAllowlisted(s_user, requestDigest), "Request should be allowlisted");
+    assertEq(s_registry.totalAllowlistedRequests(), 1, "Should have exactly 1 request in storage");
+
+    // Get the request details to verify initial expiry
+    WorkflowRegistry.OwnerAllowlistedRequest[] memory requests = s_registry.getAllowlistedRequests(0, 10);
+    assertEq(requests.length, 1, "Should return exactly 1 request");
+    assertEq(requests[0].expiryTimestamp, initialExpiry, "Initial expiry should match");
+    assertEq(requests[0].owner, s_user, "Owner should match");
+    assertEq(requests[0].requestDigest, requestDigest, "Request digest should match");
+
+    // Update the same request with new expiry (this should update in-place, not add new entry)
+    vm.expectEmit(true, true, true, false);
+    emit WorkflowRegistry.RequestAllowlisted(s_user, requestDigest, updatedExpiry);
+    vm.prank(s_user);
+    s_registry.allowlistRequest(requestDigest, updatedExpiry);
+
+    // Verify the request is still allowlisted but with updated expiry
+    assertTrue(s_registry.isRequestAllowlisted(s_user, requestDigest), "Request should still be allowlisted");
+    assertEq(s_registry.totalAllowlistedRequests(), 1, "Should still have exactly 1 request in storage (no duplicates)");
+
+    // Get the updated request details
+    requests = s_registry.getAllowlistedRequests(0, 10);
+    assertEq(requests.length, 1, "Should still return exactly 1 request");
+    assertEq(requests[0].expiryTimestamp, updatedExpiry, "Expiry should be updated");
+    assertEq(requests[0].owner, s_user, "Owner should still match");
+    assertEq(requests[0].requestDigest, requestDigest, "Request digest should still match");
+
+    // Add multiple different requests to verify they are stored separately
+    bytes32 requestDigest2 = keccak256("different-request-1");
+    bytes32 requestDigest3 = keccak256("different-request-2");
+    uint32 expiry2 = uint32(block.timestamp + 3 hours);
+    uint32 expiry3 = uint32(block.timestamp + 4 hours);
+
+    vm.prank(s_user);
+    s_registry.allowlistRequest(requestDigest2, expiry2);
+    vm.prank(s_user);
+    s_registry.allowlistRequest(requestDigest3, expiry3);
+
+    // Verify all 3 unique requests are stored
+    assertEq(s_registry.totalAllowlistedRequests(), 3, "Should have exactly 3 unique requests");
+    requests = s_registry.getAllowlistedRequests(0, 10);
+    assertEq(requests.length, 3, "Should return exactly 3 requests");
+
+    // Update the second request and verify no duplicates
+    uint32 newExpiry2 = uint32(block.timestamp + 5 hours);
+    vm.prank(s_user);
+    s_registry.allowlistRequest(requestDigest2, newExpiry2);
+
+    // Should still have exactly 3 unique requests
+    assertEq(s_registry.totalAllowlistedRequests(), 3, "Should still have exactly 3 unique requests");
+    requests = s_registry.getAllowlistedRequests(0, 10);
+    assertEq(requests.length, 3, "Should still return exactly 3 requests");
+
+    // Find and verify the updated request
+    bool foundUpdatedRequest = false;
+    for (uint256 i = 0; i < requests.length; i++) {
+      if (requests[i].requestDigest == requestDigest2) {
+        assertEq(requests[i].expiryTimestamp, newExpiry2, "Second request expiry should be updated");
+        foundUpdatedRequest = true;
+        break;
+      }
+    }
+    assertTrue(foundUpdatedRequest, "Should find the updated second request");
+  }
+
+  // When the user is linked
+  function test_allowlistRequest_WhenTheUserHasNoExistingRequest() external {
+    // It should allowlist the request digest with a new one
     bytes32 requestDigest = keccak256("request-digest");
     uint32 expiryTimestamp = uint32(block.timestamp + 1 hours);
 

contracts/src/v0.8/workflow/dev/v2/test/WorkflowRegistry/WorkflowRegistry.allowlistRequest.tree

@@ -2,4 +2,7 @@ WorkflowRegistry.allowlistRequest
 ├── when the user is not linked
 │   └── it should revert with OwnershipLinkDoesNotExist
 └── when the user is linked
-    └── it should allowlist the request digest
+    ├── when the user already has a request
+    │   └── it should allowlist the request digest by replacing the existing one
+    └── when the user has no existing request
+        └── it should allowlist the request digest with a new one

gethwrappers/workflow/generation/generated-wrapper-dependency-versions-do-not-edit.txt

@@ -1,4 +1,4 @@
 GETH_VERSION: 1.16.2
 capabilities_registry_wrapper_v2_dev: ../../contracts/solc/workflow/dev/v2/CapabilitiesRegistry/CapabilitiesRegistry.sol/CapabilitiesRegistry.abi.json ../../contracts/solc/workflow/dev/v2/CapabilitiesRegistry/CapabilitiesRegistry.sol/CapabilitiesRegistry.bin 97b005187fda8b7cc6013d92a5657d9dbf8755b36ace599de9746c29b9550e99
 workflow_registry_wrapper_v1: ../../contracts/solc/workflow/v1/WorkflowRegistry/WorkflowRegistry.sol/WorkflowRegistry.abi.json ../../contracts/solc/workflow/v1/WorkflowRegistry/WorkflowRegistry.sol/WorkflowRegistry.bin 5adc185ac7dabf6f75297855adc0d77e0bfe64af6491de944083fb10f6f9fb06
-workflow_registry_wrapper_v2_dev: ../../contracts/solc/workflow/dev/v2/WorkflowRegistry/WorkflowRegistry.sol/WorkflowRegistry.abi.json ../../contracts/solc/workflow/dev/v2/WorkflowRegistry/WorkflowRegistry.sol/WorkflowRegistry.bin 202d65bd985064c25ed347ef1f7c6cecbefe8859bda7285a87c333ef5e8ceb0c
+workflow_registry_wrapper_v2_dev: ../../contracts/solc/workflow/dev/v2/WorkflowRegistry/WorkflowRegistry.sol/WorkflowRegistry.abi.json ../../contracts/solc/workflow/dev/v2/WorkflowRegistry/WorkflowRegistry.sol/WorkflowRegistry.bin c020040c2949ef5f5de9830a11d3e340a3f3f9adcc7ded2d9588d2520d6996f1