Skip to content

Commit 3cde56f

Browse files
eutopianeutopian
committed
remove duplicates in allow listed request digests
1 parent 7e1f9b5 commit 3cde56f

File tree

3 files changed

+144
-26
lines changed

3 files changed

+144
-26
lines changed

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

Lines changed: 55 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -74,10 +74,11 @@ contract WorkflowRegistry is Ownable2StepMsgSender, ITypeAndVersion {
7474
/// When a workflow is paused it is removed from the don family.
7575
mapping(bytes32 rid => bytes32 donHash) private s_donByWorkflowRid;
7676

77-
/// @dev Tracking allowlisted requests for the owner address, required to enable anyone to verify off-chain requests.
78-
mapping(bytes32 ownerDigestHash => uint32 expiryTimestamp) private s_requestsAllowlist;
79-
/// @dev Storing allowlisted requests for all owners, enabling fetching all non-expired requests
80-
OwnerAllowlistedRequest[] private s_requestAllowlistArray;
77+
/// @dev Fast lookup for allowlisted requests. Key is keccak256(abi.encode(owner, requestDigest)), value is (array_index + 1).
78+
/// We store index+1 so that 0 can represent "not found" (since array indices start at 0).
79+
mapping(bytes32 => uint256) private s_requestIndexMap;
80+
/// @dev Array storing all allowlisted request data for enumeration and pagination.
81+
OwnerAllowlistedRequest[] private s_requestData;
8182
/// @dev Map each owner address to their arbitrary config. Can be used to control billing parameters or any other data per owner
8283
mapping(address owner => bytes config) private s_ownerConfig;
8384

@@ -1278,10 +1279,23 @@ contract WorkflowRegistry is Ownable2StepMsgSender, ITypeAndVersion {
12781279
}
12791280
if (!s_linkedOwners.contains(msg.sender)) revert OwnershipLinkDoesNotExist(msg.sender);
12801281

1281-
s_requestsAllowlist[keccak256(abi.encode(msg.sender, requestDigest))] = expiryTimestamp;
1282-
s_requestAllowlistArray.push(
1283-
OwnerAllowlistedRequest({owner: msg.sender, requestDigest: requestDigest, expiryTimestamp: expiryTimestamp})
1284-
);
1282+
bytes32 key = keccak256(abi.encode(msg.sender, requestDigest));
1283+
1284+
// Check if this request already exists (0 means not found)
1285+
uint256 storedIndex = s_requestIndexMap[key];
1286+
1287+
if (storedIndex != 0) {
1288+
// Update existing entry in place (convert back to 0-based index)
1289+
s_requestData[storedIndex - 1].expiryTimestamp = expiryTimestamp;
1290+
} else {
1291+
// Create new entry
1292+
uint256 newIndex = s_requestData.length;
1293+
s_requestData.push(
1294+
OwnerAllowlistedRequest({owner: msg.sender, requestDigest: requestDigest, expiryTimestamp: expiryTimestamp})
1295+
);
1296+
// Store index+1 so that 0 can represent "not found"
1297+
s_requestIndexMap[key] = newIndex + 1;
1298+
}
12851299
emit RequestAllowlisted(msg.sender, requestDigest, expiryTimestamp);
12861300
}
12871301

@@ -1292,41 +1306,59 @@ contract WorkflowRegistry is Ownable2StepMsgSender, ITypeAndVersion {
12921306
/// @param requestDigest Unique identifier for the request (hash of the request payload).
12931307
/// @return bool True if the request is allowlisted and not expired, false otherwise.
12941308
function isRequestAllowlisted(address owner, bytes32 requestDigest) external view returns (bool) {
1295-
return s_requestsAllowlist[keccak256(abi.encode(owner, requestDigest))] > block.timestamp;
1296-
}
1297-
1309+
bytes32 key = keccak256(abi.encode(owner, requestDigest));
1310+
uint256 storedIndex = s_requestIndexMap[key];
1311+
if (storedIndex == 0) return false; // Not found
1312+
1313+
OwnerAllowlistedRequest storage request = s_requestData[storedIndex - 1];
1314+
return request.expiryTimestamp > block.timestamp;
1315+
}
1316+
1317+
/// @notice Returns a paginated list of allowlisted requests across all owners.
1318+
/// @dev - Reads a slice of the allowlisted requests starting at `start` and spanning up to `limit` elements.
1319+
/// - Expired entries (where `expiryTimestamp <= block.timestamp`) are filtered out.
1320+
/// - The returned array may therefore be shorter than `limit`.
1321+
/// - Does not revert on out-of-range pagination: if `start >= total`, returns an empty array.
1322+
/// @param start Zero-based index into the allowlist at which to begin.
1323+
/// @param limit Maximum number of entries to return from `start`.
1324+
/// @return allowlistedRequests Array of {requestDigest, owner, expiryTimestamp} structs
1325+
/// for all non-expired requests found in the page slice.
12981326
function getAllowlistedRequests(
12991327
uint256 start,
13001328
uint256 limit
13011329
) external view returns (OwnerAllowlistedRequest[] memory allowlistedRequests) {
1302-
uint256 total = s_requestAllowlistArray.length;
1330+
uint256 total = s_requestData.length;
13031331
uint256 pageCount = _getPageCount(total, start, limit);
13041332

13051333
if (pageCount == 0) return new OwnerAllowlistedRequest[](0);
13061334

1307-
allowlistedRequests = new OwnerAllowlistedRequest[](pageCount);
1308-
uint256 addedCount = 0;
1335+
// First pass: count valid (non-expired) entries in the page range
1336+
uint256 validCount = 0;
13091337
for (uint256 i = 0; i < pageCount; ++i) {
1310-
OwnerAllowlistedRequest storage request = s_requestAllowlistArray[start + i];
1338+
OwnerAllowlistedRequest storage request = s_requestData[start + i];
13111339
if (request.expiryTimestamp > block.timestamp) {
1312-
allowlistedRequests[addedCount] = request;
1313-
++addedCount;
1340+
++validCount;
13141341
}
13151342
}
13161343

1317-
if (addedCount < pageCount) {
1318-
OwnerAllowlistedRequest[] memory shrinkedList = new OwnerAllowlistedRequest[](addedCount);
1319-
for (uint256 i = 0; i < addedCount; ++i) {
1320-
shrinkedList[i] = allowlistedRequests[i];
1344+
// Allocate exact size needed
1345+
allowlistedRequests = new OwnerAllowlistedRequest[](validCount);
1346+
1347+
// Second pass: populate the array with valid entries
1348+
uint256 addedCount = 0;
1349+
for (uint256 i = 0; i < pageCount; ++i) {
1350+
OwnerAllowlistedRequest storage request = s_requestData[start + i];
1351+
if (request.expiryTimestamp > block.timestamp) {
1352+
allowlistedRequests[addedCount] = request;
1353+
++addedCount;
13211354
}
1322-
allowlistedRequests = shrinkedList;
13231355
}
13241356

13251357
return allowlistedRequests;
13261358
}
13271359

13281360
function totalAllowlistedRequests() external view returns (uint256) {
1329-
return s_requestAllowlistArray.length;
1361+
return s_requestData.length;
13301362
}
13311363

13321364
// ================================================================

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

Lines changed: 85 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,8 +45,91 @@ contract WorkflowRegistry_allowlistRequest is WorkflowRegistrySetup {
4545
s_registry.allowlistRequest(requestDigest, expiryTimestamp);
4646
}
4747

48-
function test_allowlistRequest_WhenTheUserIsLinked() external {
49-
//it should allowlist the request digest
48+
// When the user is linked
49+
function test_allowlistRequest_WhenTheUserAlreadyHasARequest() external {
50+
// It should update the existing request in-place without growing the array
51+
bytes32 requestDigest = keccak256("duplicate-test-request");
52+
uint32 initialExpiry = uint32(block.timestamp + 1 hours);
53+
uint32 updatedExpiry = uint32(block.timestamp + 2 hours);
54+
55+
// Link the owner first
56+
_linkOwner(s_user);
57+
58+
// Initial allowlist
59+
vm.expectEmit(true, true, true, false);
60+
emit WorkflowRegistry.RequestAllowlisted(s_user, requestDigest, initialExpiry);
61+
vm.prank(s_user);
62+
s_registry.allowlistRequest(requestDigest, initialExpiry);
63+
64+
// Verify the request is allowlisted with initial expiry
65+
assertTrue(s_registry.isRequestAllowlisted(s_user, requestDigest), "Request should be allowlisted");
66+
assertEq(s_registry.totalAllowlistedRequests(), 1, "Should have exactly 1 request in storage");
67+
68+
// Get the request details to verify initial expiry
69+
WorkflowRegistry.OwnerAllowlistedRequest[] memory requests = s_registry.getAllowlistedRequests(0, 10);
70+
assertEq(requests.length, 1, "Should return exactly 1 request");
71+
assertEq(requests[0].expiryTimestamp, initialExpiry, "Initial expiry should match");
72+
assertEq(requests[0].owner, s_user, "Owner should match");
73+
assertEq(requests[0].requestDigest, requestDigest, "Request digest should match");
74+
75+
// Update the same request with new expiry (this should update in-place, not add new entry)
76+
vm.expectEmit(true, true, true, false);
77+
emit WorkflowRegistry.RequestAllowlisted(s_user, requestDigest, updatedExpiry);
78+
vm.prank(s_user);
79+
s_registry.allowlistRequest(requestDigest, updatedExpiry);
80+
81+
// Verify the request is still allowlisted but with updated expiry
82+
assertTrue(s_registry.isRequestAllowlisted(s_user, requestDigest), "Request should still be allowlisted");
83+
assertEq(s_registry.totalAllowlistedRequests(), 1, "Should still have exactly 1 request in storage (no duplicates)");
84+
85+
// Get the updated request details
86+
requests = s_registry.getAllowlistedRequests(0, 10);
87+
assertEq(requests.length, 1, "Should still return exactly 1 request");
88+
assertEq(requests[0].expiryTimestamp, updatedExpiry, "Expiry should be updated");
89+
assertEq(requests[0].owner, s_user, "Owner should still match");
90+
assertEq(requests[0].requestDigest, requestDigest, "Request digest should still match");
91+
92+
// Add multiple different requests to verify they are stored separately
93+
bytes32 requestDigest2 = keccak256("different-request-1");
94+
bytes32 requestDigest3 = keccak256("different-request-2");
95+
uint32 expiry2 = uint32(block.timestamp + 3 hours);
96+
uint32 expiry3 = uint32(block.timestamp + 4 hours);
97+
98+
vm.prank(s_user);
99+
s_registry.allowlistRequest(requestDigest2, expiry2);
100+
vm.prank(s_user);
101+
s_registry.allowlistRequest(requestDigest3, expiry3);
102+
103+
// Verify all 3 unique requests are stored
104+
assertEq(s_registry.totalAllowlistedRequests(), 3, "Should have exactly 3 unique requests");
105+
requests = s_registry.getAllowlistedRequests(0, 10);
106+
assertEq(requests.length, 3, "Should return exactly 3 requests");
107+
108+
// Update the second request and verify no duplicates
109+
uint32 newExpiry2 = uint32(block.timestamp + 5 hours);
110+
vm.prank(s_user);
111+
s_registry.allowlistRequest(requestDigest2, newExpiry2);
112+
113+
// Should still have exactly 3 unique requests
114+
assertEq(s_registry.totalAllowlistedRequests(), 3, "Should still have exactly 3 unique requests");
115+
requests = s_registry.getAllowlistedRequests(0, 10);
116+
assertEq(requests.length, 3, "Should still return exactly 3 requests");
117+
118+
// Find and verify the updated request
119+
bool foundUpdatedRequest = false;
120+
for (uint256 i = 0; i < requests.length; i++) {
121+
if (requests[i].requestDigest == requestDigest2) {
122+
assertEq(requests[i].expiryTimestamp, newExpiry2, "Second request expiry should be updated");
123+
foundUpdatedRequest = true;
124+
break;
125+
}
126+
}
127+
assertTrue(foundUpdatedRequest, "Should find the updated second request");
128+
}
129+
130+
// When the user is linked
131+
function test_allowlistRequest_WhenTheUserHasNoExistingRequest() external {
132+
// It should allowlist the request digest with a new one
50133
bytes32 requestDigest = keccak256("request-digest");
51134
uint32 expiryTimestamp = uint32(block.timestamp + 1 hours);
52135

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

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,4 +2,7 @@ WorkflowRegistry.allowlistRequest
22
├── when the user is not linked
33
│ └── it should revert with OwnershipLinkDoesNotExist
44
└── when the user is linked
5-
└── it should allowlist the request digest
5+
├── when the user already has a request
6+
│ └── it should allowlist the request digest by replacing the existing one
7+
└── when the user has no existing request
8+
└── it should allowlist the request digest with a new one

0 commit comments

Comments
 (0)