Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DEVSVCS-963] Ensure that workflow id is unique #15582

Merged
merged 3 commits into from
Dec 11, 2024
Merged

[DEVSVCS-963] Ensure that workflow id is unique #15582

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d4224b3
ensure that workflow id is unique
eutopian Dec 9, 2024
9ebdca1
Merge d4224b3ed79b33f451ab1efc858aace7d74a8190 into 88a0338576dad3406…
eutopian Dec 10, 2024
bc12ea7
Update gethwrappers
app-token-issuer-infra-releng[bot] Dec 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 14 additions & 14 deletions contracts/gas-snapshots/workflow.gas-snapshot
View file
Open in desktop
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: 422157)
WorkflowRegistry_deleteWorkflow:test_WhenTheCallerIsAnAuthorizedAddress_AndTheDonIDIsNotAllowed() (gas: 439960)
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: 515666)
39 changes: 31 additions & 8 deletions contracts/src/v0.8/workflow/dev/WorkflowRegistry.sol
View file
Open in desktop
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);
Comment on lines +302 to +303
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is redundant because above you have:

 // Condition to revert: WorkflowID must change, and at least one URL must change
    if (currentWorkflowID == newWorkflowID) {
      revert WorkflowIDNotUpdated();
    }

Now you still need to do the newWorkflowID != address(0) check, but you might be better off inlining it as it isn't quite "requireUnique".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should remove the current == old then as we still need to check globally it's unique in case it's a different but existing workflowID


// Free the old workflowID
s_workflowIDs[currentWorkflowID] = false;

// 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
s_workflowIDs[workflow.workflowID] = false;

// 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,20 @@ 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;
}

// ================================================================
// | Workflow Queries |
// ================================================================
Expand Down Expand Up @@ -636,16 +663,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
29 changes: 29 additions & 0 deletions contracts/src/v0.8/workflow/test/WorkflowRegistry/WorkflowRegistry.registerWorkflow.t.sol
View file
Open in desktop
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 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
function test_RevertWhen_TheWorkflowIDIsAlreadyInUsedByAnotherWorkflow() external {
function test_RevertWhen_TheWorkflowIDIsAlreadyInUseByAnotherWorkflow() external {

typo

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
2 changes: 2 additions & 0 deletions contracts/src/v0.8/workflow/test/WorkflowRegistry/WorkflowRegistry.registerWorkflow.tree
View file
Open in desktop
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
26 changes: 26 additions & 0 deletions contracts/src/v0.8/workflow/test/WorkflowRegistry/WorkflowRegistry.updateWorkflow.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,32 @@ 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.startPrank(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.expectRevert(WorkflowRegistry.WorkflowIDAlreadyExists.selector);
s_registry.updateWorkflow(
s_validWorkflowKey, s_newValidWorkflowID, s_validBinaryURL, s_validConfigURL, s_newValidSecretsURL
);

vm.stopPrank();
}

// whenTheCallerIsAnAuthorizedAddress whenTheRegistryIsNotLocked whenTheDonIDIsAllowed whenTheCallerIsTheWorkflowOwner
function test_WhenTheWorkflowInputsAreAllValid() external {
// Register a workflow first.
Expand Down
2 changes: 2 additions & 0 deletions contracts/src/v0.8/workflow/test/WorkflowRegistry/WorkflowRegistry.updateWorkflow.tree
View file
Open in desktop
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
Loading
Loading