Workflow Registry: Add remaining tests for workflow registry manager (smartcontractkit#15359)

* add remaining tests for workflow registry manager

* update geth wrappers

* address comments

Author: eutopian

.github/workflows/solidity-foundry.yml

@@ -40,7 +40,7 @@ jobs:
             { "name": "shared", "setup": { "run-coverage": true, "extra-coverage-params": "--no-match-path='*CallWithExactGas*' --ir-minimum", "min-coverage": 32.6, "run-gas-snapshot": true, "run-forge-fmt": false }},
             { "name": "transmission", "setup": { "run-coverage": true, "min-coverage": 61.5, "run-gas-snapshot": true, "run-forge-fmt": false }},
             { "name": "vrf", "setup": { "run-coverage": false, "min-coverage": 98.5, "run-gas-snapshot": false, "run-forge-fmt": false }},
-            { "name": "workflow", "setup": { "run-coverage": true, "extra-coverage-params": "--ir-minimum", "min-coverage": 65.0, "run-gas-snapshot": false, "run-forge-fmt": true }}
+            { "name": "workflow", "setup": { "run-coverage": true, "extra-coverage-params": "--ir-minimum", "min-coverage": 96.0, "run-gas-snapshot": true, "run-forge-fmt": true }}
           ]
           EOF
 

contracts/gas-snapshots/workflow.gas-snapshot

@@ -1,53 +1,64 @@
-WorkflowRegistryManager_activateVersion:test_WhenTheVersionNumberIsNotActive() (gas: 419)
-WorkflowRegistryManageraddVersion:test_WhenAutoActivateIsFalse() (gas: 164)
-WorkflowRegistryManageraddVersion:test_WhenAutoActivateIsTrue() (gas: 120)
-WorkflowRegistryManagergetActiveVersion:test_WhenAnActiveVersionExists() (gas: 120)
-WorkflowRegistryManagergetActiveVersion:test_WhenNoActiveVersionIsAvailable() (gas: 139)
-WorkflowRegistryManagergetAllVersions:test_WhenLimitExceedsMaximumPaginationLimit() (gas: 161)
-WorkflowRegistryManagergetAllVersions:test_WhenRequestingWithInvalidStartIndex() (gas: 142)
-WorkflowRegistryManagergetAllVersions:test_WhenRequestingWithValidStartIndexAndLimitWithinBounds() (gas: 120)
-WorkflowRegistryManagergetLatestVersion:test_WhenNoVersionsHaveBeenRegistered() (gas: 120)
-WorkflowRegistryManagergetLatestVersion:test_WhenVersionsHaveBeenRegistered() (gas: 139)
-WorkflowRegistryManagergetVersion:test_WhenVersionNumberIsNotRegistered() (gas: 120)
-WorkflowRegistryManagergetVersion:test_WhenVersionNumberIsRegistered() (gas: 139)
-WorkflowRegistryManagergetVersionNumber:test_WhenAVersionIsRegisteredForTheContractAddressAndChainIDCombination() (gas: 161)
-WorkflowRegistryManagergetVersionNumber:test_WhenNoVersionIsRegisteredForTheContractAddressAndChainIDCombination() (gas: 120)
-WorkflowRegistryManagergetVersionNumber:test_WhenTheContractAddressIsInvalid() (gas: 142)
-WorkflowRegistry_activateWorkflow:test_WhenTheCallerIsAnAuthorizedAddress() (gas: 491327)
-WorkflowRegistry_deleteWorkflow:test_WhenTheCallerIsAnAuthorizedAddress_AndTheDonIDIsAllowed() (gas: 400597)
-WorkflowRegistry_deleteWorkflow:test_WhenTheCallerIsAnAuthorizedAddress_AndTheDonIDIsNotAllowed() (gas: 418579)
+WorkflowRegistryManager_activateVersion:test_WhenTheVersionNumberIsNotActive_AndWhenThereAreNoActiveVersions() (gas: 528769)
+WorkflowRegistryManager_addVersion:test_WhenAutoActivateIsFalse() (gas: 265551)
+WorkflowRegistryManager_addVersion:test_WhenAutoActivateIsTrue() (gas: 270596)
+WorkflowRegistryManager_getActiveVersion:test_WhenAnActiveVersionExists() (gas: 287760)
+WorkflowRegistryManager_getActiveVersion:test_WhenNoActiveVersionIsAvailable() (gas: 13258)
+WorkflowRegistryManager_getActiveVersionNumber:test_WhenAnActiveVersionExists() (gas: 283885)
+WorkflowRegistryManager_getActiveVersionNumber:test_WhenNoActiveVersionIsAvailable() (gas: 10698)
+WorkflowRegistryManager_getAllVersions:test_WhenLimitExceedsMaximumPaginationLimit() (gas: 54503)
+WorkflowRegistryManager_getAllVersions:test_WhenRequestingWithInvalidStartIndex() (gas: 11338)
+WorkflowRegistryManager_getAllVersions:test_WhenRequestingWithValidStartIndexAndLimitWithinBounds() (gas: 40398)
+WorkflowRegistryManager_getLatestVersion:test_WhenNoVersionsHaveBeenRegistered() (gas: 12984)
+WorkflowRegistryManager_getLatestVersion:test_WhenVersionsHaveBeenRegistered() (gas: 287791)
+WorkflowRegistryManager_getLatestVersionNumber:test_WhenNoVersionsHaveBeenRegistered() (gas: 10637)
+WorkflowRegistryManager_getLatestVersionNumber:test_WhenVersionsHaveBeenRegistered() (gas: 284134)
+WorkflowRegistryManager_getVersion:test_WhenVersionNumberIsNotRegistered() (gas: 13785)
+WorkflowRegistryManager_getVersion:test_WhenVersionNumberIsRegistered() (gas: 288169)
+WorkflowRegistryManager_getVersionNumberByContractAddressAndChainID:test_WhenAVersionIsRegisteredForTheContractAddressAndChainIDCombination() (gas: 285022)
+WorkflowRegistryManager_getVersionNumberByContractAddressAndChainID:test_WhenNoVersionIsRegisteredForTheContractAddressAndChainIDCombination() (gas: 286634)
+WorkflowRegistryManager_getVersionNumberByContractAddressAndChainID:test_WhenTheContractAddressIsInvalid() (gas: 284604)
+WorkflowRegistry_activateWorkflow:test_WhenTheCallerIsAnAuthorizedAddress() (gas: 495029)
+WorkflowRegistry_deleteWorkflow:test_WhenTheCallerIsAnAuthorizedAddress_AndTheDonIDIsAllowed() (gas: 403945)
+WorkflowRegistry_deleteWorkflow:test_WhenTheCallerIsAnAuthorizedAddress_AndTheDonIDIsNotAllowed() (gas: 421748)
+WorkflowRegistry_getAllAllowedDONs:test_WhenTheRegistryIsLocked() (gas: 47473)
 WorkflowRegistry_getAllAllowedDONs:test_WhenTheSetOfAllowedDONsIsEmpty() (gas: 25780)
 WorkflowRegistry_getAllAllowedDONs:test_WhenThereAreMultipleAllowedDONs() (gas: 75437)
-WorkflowRegistry_getAllAllowedDONs:test_WhenThereIsASingleAllowedDON() (gas: 16566)
-WorkflowRegistry_getWorkflowMetadata:test_WhenTheWorkflowDoesNotExist() (gas: 17521)
-WorkflowRegistry_getWorkflowMetadata:test_WhenTheWorkflowExistsWithTheOwnerAndName() (gas: 490176)
-WorkflowRegistry_getWorkflowMetadataListByDON:test_WhenLimitExceedsTotalWorkflows() (gas: 127660)
-WorkflowRegistry_getWorkflowMetadataListByDON:test_WhenLimitIsEqualToTotalWorkflows() (gas: 128013)
-WorkflowRegistry_getWorkflowMetadataListByDON:test_WhenLimitIsLessThanTotalWorkflows() (gas: 90119)
-WorkflowRegistry_getWorkflowMetadataListByDON:test_WhenStartIs0() (gas: 127572)
-WorkflowRegistry_getWorkflowMetadataListByDON:test_WhenStartIsGreaterThan0() (gas: 90076)
-WorkflowRegistry_getWorkflowMetadataListByDON:test_WhenStartIsGreaterThanOrEqualToTotalWorkflows() (gas: 13454)
+WorkflowRegistry_getAllAllowedDONs:test_WhenThereIsASingleAllowedDON() (gas: 16590)
+WorkflowRegistry_getAllAuthorizedAddresses:test_WhenTheRegistryIsLocked() (gas: 47740)
+WorkflowRegistry_getAllAuthorizedAddresses:test_WhenTheSetOfAuthorizedAddressesIsEmpty() (gas: 26152)
+WorkflowRegistry_getAllAuthorizedAddresses:test_WhenThereAreMultipleAuthorizedAddresses() (gas: 78270)
+WorkflowRegistry_getAllAuthorizedAddresses:test_WhenThereIsASingleAuthorizedAddress() (gas: 16832)
+WorkflowRegistry_getWorkflowMetadata:test_WhenTheRegistryIsLocked() (gas: 519145)
+WorkflowRegistry_getWorkflowMetadata:test_WhenTheWorkflowDoesNotExist() (gas: 17543)
+WorkflowRegistry_getWorkflowMetadata:test_WhenTheWorkflowExistsWithTheOwnerAndName() (gas: 490001)
+WorkflowRegistry_getWorkflowMetadataListByDON:test_WhenLimitExceedsTotalWorkflows() (gas: 128146)
+WorkflowRegistry_getWorkflowMetadataListByDON:test_WhenLimitIsEqualToTotalWorkflows() (gas: 128035)
+WorkflowRegistry_getWorkflowMetadataListByDON:test_WhenLimitIsLessThanTotalWorkflows() (gas: 90141)
+WorkflowRegistry_getWorkflowMetadataListByDON:test_WhenStartIs0() (gas: 128075)
+WorkflowRegistry_getWorkflowMetadataListByDON:test_WhenStartIsGreaterThan0() (gas: 90098)
+WorkflowRegistry_getWorkflowMetadataListByDON:test_WhenStartIsGreaterThanOrEqualToTotalWorkflows() (gas: 13476)
 WorkflowRegistry_getWorkflowMetadataListByDON:test_WhenTheDONHasNoWorkflows() (gas: 13410)
-WorkflowRegistry_getWorkflowMetadataListByOwner:test_WhenLimitExceedsTotalWorkflows() (gas: 128221)
-WorkflowRegistry_getWorkflowMetadataListByOwner:test_WhenLimitIsEqualToTotalWorkflows() (gas: 128320)
-WorkflowRegistry_getWorkflowMetadataListByOwner:test_WhenLimitIsLessThanTotalWorkflows() (gas: 90404)
-WorkflowRegistry_getWorkflowMetadataListByOwner:test_WhenStartIs0_AndLimitIs0() (gas: 128118)
-WorkflowRegistry_getWorkflowMetadataListByOwner:test_WhenStartIsGreaterThan0() (gas: 90361)
+WorkflowRegistry_getWorkflowMetadataListByDON:test_WhenTheRegistryIsLocked() (gas: 158899)
+WorkflowRegistry_getWorkflowMetadataListByOwner:test_WhenLimitExceedsTotalWorkflows() (gas: 135174)
+WorkflowRegistry_getWorkflowMetadataListByOwner:test_WhenLimitIsEqualToTotalWorkflows() (gas: 135073)
+WorkflowRegistry_getWorkflowMetadataListByOwner:test_WhenLimitIsLessThanTotalWorkflows() (gas: 95635)
+WorkflowRegistry_getWorkflowMetadataListByOwner:test_WhenStartIs0() (gas: 135113)
+WorkflowRegistry_getWorkflowMetadataListByOwner:test_WhenStartIsGreaterThan0() (gas: 95592)
 WorkflowRegistry_getWorkflowMetadataListByOwner:test_WhenStartIsGreaterThanOrEqualToTotalWorkflows() (gas: 13764)
-WorkflowRegistry_getWorkflowMetadataListByOwner:test_WhenTheOwnerHasNoWorkflows() (gas: 13984)
-WorkflowRegistry_pauseWorkflow:test_WhenTheDonIDIsAllowed_AndTheCallerIsAnAuthorizedAddress() (gas: 491299)
-WorkflowRegistry_pauseWorkflow:test_WhenTheDonIDIsAllowed_AndTheCallerIsAnUnauthorizedAddress() (gas: 499097)
-WorkflowRegistry_pauseWorkflow:test_WhenTheDonIDIsNotAllowed_AndTheCallerIsAnAuthorizedAddress() (gas: 498850)
-WorkflowRegistry_pauseWorkflow:test_WhenTheDonIDIsNotAllowed_AndTheCallerIsAnUnauthorizedAddress() (gas: 503264)
-WorkflowRegistry_registerWorkflow:test_WhenTheWorkflowInputsAreAllValid() (gas: 549829)
+WorkflowRegistry_getWorkflowMetadataListByOwner:test_WhenTheOwnerHasNoWorkflows() (gas: 14006)
+WorkflowRegistry_getWorkflowMetadataListByOwner:test_WhenTheRegistryIsLocked() (gas: 165968)
+WorkflowRegistry_lockRegistry:test_WhenTheCallerIsTheContractOwner() (gas: 38758)
+WorkflowRegistry_pauseWorkflow:test_WhenTheDonIDIsAllowed_AndTheCallerIsAnAuthorizedAddress() (gas: 494993)
+WorkflowRegistry_pauseWorkflow:test_WhenTheDonIDIsAllowed_AndTheCallerIsAnUnauthorizedAddress() (gas: 502796)
+WorkflowRegistry_pauseWorkflow:test_WhenTheDonIDIsNotAllowed_AndTheCallerIsAnAuthorizedAddress() (gas: 502555)
+WorkflowRegistry_pauseWorkflow:test_WhenTheDonIDIsNotAllowed_AndTheCallerIsAnUnauthorizedAddress() (gas: 506966)
+WorkflowRegistry_registerWorkflow:test_WhenTheWorkflowInputsAreAllValid() (gas: 549769)
 WorkflowRegistry_requestForceUpdateSecrets:test_WhenTheCallerIsAnAuthorizedAddress_AndTheWorkflowIsInAnAllowedDON() (gas: 891242)
 WorkflowRegistry_requestForceUpdateSecrets:test_WhenTheCallerIsAnAuthorizedAddress_AndTheWorkflowIsNotInAnAllowedDON() (gas: 488397)
 WorkflowRegistry_requestForceUpdateSecrets:test_WhenTheCallerIsNotAnAuthorizedAddress() (gas: 486751)
-WorkflowRegistry_updateAllowedDONs:test_WhenTheBoolInputIsFalse() (gas: 29811)
-WorkflowRegistry_updateAllowedDONs:test_WhenTheBoolInputIsTrue() (gas: 170386)
-WorkflowRegistry_updateAuthorizedAddresses:test_WhenTheBoolInputIsFalse() (gas: 30350)
-WorkflowRegistry_updateAuthorizedAddresses:test_WhenTheBoolInputIsTrue() (gas: 175605)
-WorkflowRegistry_updateWorkflow:test_WhenTheWorkflowInputsAreAllValid() (gas: 501589)
-WorkflowRegistrygetAllAuthorizedAddresses:test_WhenTheSetOfAuthorizedAddressesIsEmpty() (gas: 26152)
-WorkflowRegistrygetAllAuthorizedAddresses:test_WhenThereAreMultipleAuthorizedAddresses() (gas: 78270)
-WorkflowRegistrygetAllAuthorizedAddresses:test_WhenThereIsASingleAuthorizedAddress() (gas: 16814)
+WorkflowRegistry_unlockRegistry:test_WhenTheCallerIsTheContractOwner() (gas: 30325)
+WorkflowRegistry_updateAllowedDONs:test_WhenTheBoolInputIsFalse() (gas: 29739)
+WorkflowRegistry_updateAllowedDONs:test_WhenTheBoolInputIsTrue() (gas: 170296)
+WorkflowRegistry_updateAuthorizedAddresses:test_WhenTheBoolInputIsFalse() (gas: 30278)
+WorkflowRegistry_updateAuthorizedAddresses:test_WhenTheBoolInputIsTrue() (gas: 175515)
+WorkflowRegistry_updateWorkflow:test_WhenTheWorkflowInputsAreAllValid() (gas: 479601)

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

@@ -83,8 +83,8 @@ contract WorkflowRegistry is Ownable2StepMsgSender, ITypeAndVersion {
     bytes32 indexed workflowID, address indexed workflowOwner, uint32 indexed donID, string workflowName
   );
   event WorkflowForceUpdateSecretsRequestedV1(address indexed owner, bytes32 secretsURLHash, string workflowName);
-  event RegistryLockedV1(address indexed lockedBy);
-  event RegistryUnlockedV1(address indexed unlockedBy);
+  event RegistryLockedV1(address lockedBy);
+  event RegistryUnlockedV1(address unlockedBy);
 
   error AddressNotAuthorized(address caller);
   error CallerIsNotWorkflowOwner(address caller);
@@ -306,7 +306,7 @@ contract WorkflowRegistry is Ownable2StepMsgSender, ITypeAndVersion {
     if (!sameSecretsURL) {
       // Remove the old secrets hash if secretsURL is not empty
       if (bytes(workflow.secretsURL).length > 0) {
-        // Using keccak256 instead of _computeOwnerAndStringFieldHashKey as currentSecretsURL is memory
+        // Using keccak256 instead of computeHashKey as currentSecretsURL is memory
         bytes32 oldSecretsHash = keccak256(abi.encodePacked(msg.sender, workflow.secretsURL));
         s_secretsHashToWorkflows[oldSecretsHash].remove(workflowKey);
       }
@@ -378,34 +378,31 @@ contract WorkflowRegistry is Ownable2StepMsgSender, ITypeAndVersion {
   function deleteWorkflow(
     bytes32 workflowKey
   ) external registryNotLocked {
-    address sender = msg.sender;
-
     // Retrieve workflow metadata from storage
-    WorkflowMetadata storage workflow = _getWorkflowFromStorage(sender, workflowKey);
-    uint32 donID = workflow.donID;
+    WorkflowMetadata storage workflow = _getWorkflowFromStorage(msg.sender, workflowKey);
 
     // Only checking access for the caller instead of using _validatePermissions so that even if the DON was removed from the
     // allowed list, the workflow can still be deleted.
-    if (!s_authorizedAddresses.contains(sender)) {
-      revert AddressNotAuthorized(sender);
+    if (!s_authorizedAddresses.contains(msg.sender)) {
+      revert AddressNotAuthorized(msg.sender);
     }
 
     // Remove the workflow from the owner and DON mappings
-    s_ownerWorkflowKeys[sender].remove(workflowKey);
-    s_donWorkflowKeys[donID].remove(workflowKey);
+    s_ownerWorkflowKeys[msg.sender].remove(workflowKey);
+    s_donWorkflowKeys[workflow.donID].remove(workflowKey);
 
     // Remove the workflow from the secrets hash set if secretsURL is not empty
     if (bytes(workflow.secretsURL).length > 0) {
-      // Using keccak256 instead of _computeOwnerAndStringFieldHashKey as secretsURL is storage ref
-      bytes32 secretsHash = keccak256(abi.encodePacked(sender, workflow.secretsURL));
+      // Using keccak256 instead of computeHashKey as secretsURL is storage ref
+      bytes32 secretsHash = keccak256(abi.encodePacked(msg.sender, workflow.secretsURL));
       s_secretsHashToWorkflows[secretsHash].remove(workflowKey);
     }
 
+    // Emit an event indicating the workflow has been deleted. We need to do this before deleting the workflow from storage.
+    emit WorkflowDeletedV1(workflow.workflowID, msg.sender, workflow.donID, workflow.workflowName);
+
     // Delete the workflow metadata from storage
     delete s_workflows[workflowKey];
-
-    // Emit an event indicating the workflow has been deleted
-    emit WorkflowDeletedV1(workflow.workflowID, sender, donID, workflow.workflowName);
   }
 
   /// @notice Requests a force update for workflows that share the same secrets URL.
@@ -430,10 +427,8 @@ contract WorkflowRegistry is Ownable2StepMsgSender, ITypeAndVersion {
   function requestForceUpdateSecrets(
     string calldata secretsURL
   ) external registryNotLocked {
-    address sender = msg.sender;
-
     // Use secretsURL and sender hash key to get the mapping key
-    bytes32 secretsHash = computeHashKey(sender, secretsURL);
+    bytes32 secretsHash = computeHashKey(msg.sender, secretsURL);
 
     // Retrieve all workflow keys associated with the given secrets hash
     EnumerableSet.Bytes32Set storage workflowKeys = s_secretsHashToWorkflows[secretsHash];
@@ -449,8 +444,8 @@ contract WorkflowRegistry is Ownable2StepMsgSender, ITypeAndVersion {
       bytes32 workflowKey = workflowKeys.at(i);
       WorkflowMetadata storage workflow = s_workflows[workflowKey];
 
-      if (s_allowedDONs.contains(workflow.donID) && s_authorizedAddresses.contains(sender)) {
-        emit WorkflowForceUpdateSecretsRequestedV1(sender, secretsHash, workflow.workflowName);
+      if (s_allowedDONs.contains(workflow.donID) && s_authorizedAddresses.contains(msg.sender)) {
+        emit WorkflowForceUpdateSecretsRequestedV1(msg.sender, secretsHash, workflow.workflowName);
       }
     }
   }
@@ -472,10 +467,8 @@ contract WorkflowRegistry is Ownable2StepMsgSender, ITypeAndVersion {
   /// @param workflowKey The unique identifier for the workflow.
   /// @param newStatus The new status to set for the workflow (either `Paused` or `Active`).
   function _updateWorkflowStatus(bytes32 workflowKey, WorkflowStatus newStatus) internal {
-    address sender = msg.sender;
-
     // Retrieve workflow metadata once
-    WorkflowMetadata storage workflow = _getWorkflowFromStorage(sender, workflowKey);
+    WorkflowMetadata storage workflow = _getWorkflowFromStorage(msg.sender, workflowKey);
     uint32 donID = workflow.donID;
 
     // Avoid unnecessary storage writes if already in the desired status
@@ -485,17 +478,17 @@ contract WorkflowRegistry is Ownable2StepMsgSender, ITypeAndVersion {
 
     // Check if the DON ID is allowed when activating a workflow
     if (newStatus == WorkflowStatus.ACTIVE) {
-      _validatePermissions(donID, sender);
+      _validatePermissions(donID, msg.sender);
     }
 
     // Update the workflow status
     workflow.status = newStatus;
 
     // Emit the appropriate event based on newStatus
     if (newStatus == WorkflowStatus.PAUSED) {
-      emit WorkflowPausedV1(workflow.workflowID, sender, donID, workflow.workflowName);
+      emit WorkflowPausedV1(workflow.workflowID, msg.sender, donID, workflow.workflowName);
     } else if (newStatus == WorkflowStatus.ACTIVE) {
-      emit WorkflowActivatedV1(workflow.workflowID, sender, donID, workflow.workflowName);
+      emit WorkflowActivatedV1(workflow.workflowID, msg.sender, donID, workflow.workflowName);
     }
   }