Skip to content

Commit

Permalink
ensure that workflow id is unique
Browse files Browse the repository at this point in the history
  • Loading branch information
eutopian committed Dec 10, 2024
1 parent 45898fc commit 4778956
Show file tree
Hide file tree
Showing 6 changed files with 111 additions and 22 deletions.
28 changes: 14 additions & 14 deletions contracts/gas-snapshots/workflow.gas-snapshot
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,9 @@ WorkflowRegistryManager_getVersion:test_WhenVersionNumberIsRegistered() (gas: 28
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_activateWorkflow:test_WhenTheCallerIsAnAuthorizedAddress() (gas: 517416)
WorkflowRegistry_deleteWorkflow:test_WhenTheCallerIsAnAuthorizedAddress_AndTheDonIDIsAllowed() (gas: 422167)
WorkflowRegistry_deleteWorkflow:test_WhenTheCallerIsAnAuthorizedAddress_AndTheDonIDIsNotAllowed() (gas: 439969)
WorkflowRegistry_getAllAllowedDONs:test_WhenTheRegistryIsLocked() (gas: 47473)
WorkflowRegistry_getAllAllowedDONs:test_WhenTheSetOfAllowedDONsIsEmpty() (gas: 25780)
WorkflowRegistry_getAllAllowedDONs:test_WhenThereAreMultipleAllowedDONs() (gas: 75437)
Expand All @@ -28,9 +28,9 @@ WorkflowRegistry_getAllAuthorizedAddresses:test_WhenTheRegistryIsLocked() (gas:
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_WhenTheRegistryIsLocked() (gas: 541532)
WorkflowRegistry_getWorkflowMetadata:test_WhenTheWorkflowDoesNotExist() (gas: 17543)
WorkflowRegistry_getWorkflowMetadata:test_WhenTheWorkflowExistsWithTheOwnerAndName() (gas: 490001)
WorkflowRegistry_getWorkflowMetadata:test_WhenTheWorkflowExistsWithTheOwnerAndName() (gas: 512388)
WorkflowRegistry_getWorkflowMetadataListByDON:test_WhenLimitExceedsTotalWorkflows() (gas: 128146)
WorkflowRegistry_getWorkflowMetadataListByDON:test_WhenLimitIsEqualToTotalWorkflows() (gas: 128035)
WorkflowRegistry_getWorkflowMetadataListByDON:test_WhenLimitIsLessThanTotalWorkflows() (gas: 90141)
Expand All @@ -48,17 +48,17 @@ WorkflowRegistry_getWorkflowMetadataListByOwner:test_WhenStartIsGreaterThanOrEqu
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_pauseWorkflow:test_WhenTheDonIDIsAllowed_AndTheCallerIsAnAuthorizedAddress() (gas: 517380)
WorkflowRegistry_pauseWorkflow:test_WhenTheDonIDIsAllowed_AndTheCallerIsAnUnauthorizedAddress() (gas: 525183)
WorkflowRegistry_pauseWorkflow:test_WhenTheDonIDIsNotAllowed_AndTheCallerIsAnAuthorizedAddress() (gas: 524942)
WorkflowRegistry_pauseWorkflow:test_WhenTheDonIDIsNotAllowed_AndTheCallerIsAnUnauthorizedAddress() (gas: 529353)
WorkflowRegistry_registerWorkflow:test_WhenTheWorkflowInputsAreAllValid() (gas: 572178)
WorkflowRegistry_requestForceUpdateSecrets:test_WhenTheCallerIsAnAuthorizedAddress_AndTheWorkflowIsInAnAllowedDON() (gas: 936016)
WorkflowRegistry_requestForceUpdateSecrets:test_WhenTheCallerIsAnAuthorizedAddress_AndTheWorkflowIsNotInAnAllowedDON() (gas: 510784)
WorkflowRegistry_requestForceUpdateSecrets:test_WhenTheCallerIsNotAnAuthorizedAddress() (gas: 509138)
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)
WorkflowRegistry_updateWorkflow:test_WhenTheWorkflowInputsAreAllValid() (gas: 515706)
47 changes: 39 additions & 8 deletions contracts/src/v0.8/workflow/dev/WorkflowRegistry.sol
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ contract WorkflowRegistry is Ownable2StepMsgSender, ITypeAndVersion {
/// @dev Mapping to track workflows by secretsURL hash (owner + secretsURL).
/// This is used to find all workflows that have the same secretsURL when a force secrets update event is requested.
mapping(bytes32 secretsURLHash => EnumerableSet.Bytes32Set workflowKeys) private s_secretsHashToWorkflows;
/// @dev Keep track of all workflowIDs to ensure uniqueness.
mapping(bytes32 workflowID => bool inUse) private s_workflowIDs;

/// @dev List of all authorized EOAs/contracts allowed to access this contract's state functions. All view functions are open access.
EnumerableSet.AddressSet private s_authorizedAddresses;
Expand Down Expand Up @@ -203,13 +205,15 @@ contract WorkflowRegistry is Ownable2StepMsgSender, ITypeAndVersion {
) external registryNotLocked {
_validatePermissions(donID, msg.sender);
_validateWorkflowName(bytes(workflowName).length);
_validateWorkflowMetadata(workflowID, bytes(binaryURL).length, bytes(configURL).length, bytes(secretsURL).length);
_validateWorkflowURLs(bytes(binaryURL).length, bytes(configURL).length, bytes(secretsURL).length);

bytes32 workflowKey = computeHashKey(msg.sender, workflowName);
if (s_workflows[workflowKey].owner != address(0)) {
revert WorkflowAlreadyRegistered();
}

_requireUniqueWorkflowID(workflowID);

// Create new workflow entry
s_workflows[workflowKey] = WorkflowMetadata({
workflowID: workflowID,
Expand Down Expand Up @@ -272,7 +276,7 @@ contract WorkflowRegistry is Ownable2StepMsgSender, ITypeAndVersion {
string calldata configURL,
string calldata secretsURL
) external registryNotLocked {
_validateWorkflowMetadata(newWorkflowID, bytes(binaryURL).length, bytes(configURL).length, bytes(secretsURL).length);
_validateWorkflowURLs(bytes(binaryURL).length, bytes(configURL).length, bytes(secretsURL).length);

WorkflowMetadata storage workflow = _getWorkflowFromStorage(msg.sender, workflowKey);

Expand All @@ -295,6 +299,12 @@ contract WorkflowRegistry is Ownable2StepMsgSender, ITypeAndVersion {
revert WorkflowContentNotUpdated();
}

// Ensure the new workflowID is unique
_requireUniqueWorkflowID(newWorkflowID);

// Free the old workflowID
_releaseWorkflowID(currentWorkflowID);

// Update all fields that have changed and the relevant sets
workflow.workflowID = newWorkflowID;
if (!sameBinaryURL) {
Expand Down Expand Up @@ -387,6 +397,9 @@ contract WorkflowRegistry is Ownable2StepMsgSender, ITypeAndVersion {
revert AddressNotAuthorized(msg.sender);
}

// Release the workflowID for reuse
_releaseWorkflowID(workflow.workflowID);

// Remove the workflow from the owner and DON mappings
s_ownerWorkflowKeys[msg.sender].remove(workflowKey);
s_donWorkflowKeys[workflow.donID].remove(workflowKey);
Expand Down Expand Up @@ -508,6 +521,28 @@ contract WorkflowRegistry is Ownable2StepMsgSender, ITypeAndVersion {
return workflow;
}

/// @notice Ensures the given workflowID is unique and marks it as used.
/// @param workflowID The workflowID to validate and consume.
function _requireUniqueWorkflowID(
bytes32 workflowID
) internal {
if (workflowID == bytes32(0)) revert InvalidWorkflowID();

if (s_workflowIDs[workflowID]) {
revert WorkflowIDAlreadyExists();
}

s_workflowIDs[workflowID] = true;
}

/// @notice Frees up a previously used workflowID, allowing it to be reused.
/// @param workflowID The workflowID to release.
function _releaseWorkflowID(
bytes32 workflowID
) internal {
s_workflowIDs[workflowID] = false;
}

// ================================================================
// | Workflow Queries |
// ================================================================
Expand Down Expand Up @@ -636,16 +671,12 @@ contract WorkflowRegistry is Ownable2StepMsgSender, ITypeAndVersion {
// | Validation |
// ================================================================

/// @dev Internal function to validate the metadata for a workflow.
/// @param workflowID The unique identifier for the workflow.
function _validateWorkflowMetadata(
bytes32 workflowID,
/// @dev Internal function to validate the urls for a workflow.
function _validateWorkflowURLs(
uint256 binaryURLLength,
uint256 configURLLength,
uint256 secretsURLLength
) internal pure {
if (workflowID == bytes32(0)) revert InvalidWorkflowID();

if (binaryURLLength > MAX_URL_LENGTH) {
revert URLTooLong(binaryURLLength, MAX_URL_LENGTH);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,35 @@ contract WorkflowRegistry_registerWorkflow is WorkflowRegistrySetup {
);
}

// whenTheCallerIsAnAuthorizedAddress whenTheRegistryIsNotLocked whenTheDonIDIsAllowed
function test_RevertWhen_TheWorkflowIDIsAlreadyInUsedByAnotherWorkflow() external {
vm.startPrank(s_authorizedAddress);

// Register a valid workflow first
s_registry.registerWorkflow(
s_validWorkflowName,
s_validWorkflowID,
s_allowedDonID,
WorkflowRegistry.WorkflowStatus.ACTIVE,
s_validBinaryURL,
s_validConfigURL,
s_validSecretsURL
);

vm.expectRevert(WorkflowRegistry.WorkflowIDAlreadyExists.selector);
s_registry.registerWorkflow(
"ValidWorkflow2",
s_validWorkflowID,
s_allowedDonID,
WorkflowRegistry.WorkflowStatus.ACTIVE,
s_validBinaryURL,
s_validConfigURL,
s_validSecretsURL
);

vm.stopPrank();
}

// whenTheCallerIsAnAuthorizedAddress whenTheRegistryIsNotLocked whenTheDonIDIsAllowed
function test_RevertWhen_TheWorkflowNameIsAlreadyUsedByTheOwner() external {
vm.startPrank(s_authorizedAddress);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ WorkflowRegistry.registerWorkflow
│ └── it should revert
├── when the workflowID is invalid
│ └── it should revert
├── when the workflowID is already in used by another workflow
│ └── it should revert
├── when the workflow name is already used by the owner
│ └── it should revert
└── when the workflow inputs are all valid
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,31 @@ contract WorkflowRegistry_updateWorkflow is WorkflowRegistrySetup {
s_registry.updateWorkflow(s_validWorkflowKey, bytes32(0), s_validBinaryURL, s_validConfigURL, s_newValidSecretsURL);
}

// whenTheCallerIsAnAuthorizedAddress whenTheRegistryIsNotLocked whenTheDonIDIsAllowed whenTheCallerIsTheWorkflowOwner
function test_RevertWhen_TheWorkflowIDIsAlreadyInUsedByAnotherWorkflow() external {
// Register a workflow first
_registerValidWorkflow();

// Register another workflow with another workflow ID
vm.prank(s_authorizedAddress);
s_registry.registerWorkflow(
"ValidWorkflow2",
s_newValidWorkflowID,
s_allowedDonID,
WorkflowRegistry.WorkflowStatus.ACTIVE,
s_validBinaryURL,
s_validConfigURL,
s_validSecretsURL
);

// Update the workflow with a workflow ID that is already in use by another workflow.
vm.prank(s_authorizedAddress);
vm.expectRevert(WorkflowRegistry.WorkflowIDAlreadyExists.selector);
s_registry.updateWorkflow(
s_validWorkflowKey, s_newValidWorkflowID, s_validBinaryURL, s_validConfigURL, s_newValidSecretsURL
);
}

// whenTheCallerIsAnAuthorizedAddress whenTheRegistryIsNotLocked whenTheDonIDIsAllowed whenTheCallerIsTheWorkflowOwner
function test_WhenTheWorkflowInputsAreAllValid() external {
// Register a workflow first.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ WorkflowRegistry.updateWorkflow
│ └── it should revert
├── when the workflowID is invalid
│ └── it should revert
├── when the workflowID is already in used by another workflow
│ └── it should revert
└── when the workflow inputs are all valid
├── it should update the existing workflow in s_workflows with the new values
├── it should emit {WorkflowUpdatedV1}
Expand Down

0 comments on commit 4778956

Please sign in to comment.