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

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

Merged
merged 3 commits into from
Nov 25, 2024
Merged

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

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
c6bed1a
add remaining tests for workflow registry manager
eutopian Nov 21, 2024
8e9b0ee
update geth wrappers
eutopian Nov 21, 2024
176a001
address comments
eutopian Nov 21, 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
2 changes: 1 addition & 1 deletion .github/workflows/solidity-foundry.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
103 changes: 57 additions & 46 deletions contracts/gas-snapshots/workflow.gas-snapshot
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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)
47 changes: 20 additions & 27 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 @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

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

why removing these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The event is highly infrequent and the index won't help much with anything except additional gas cost

Copy link
Contributor

Choose a reason for hiding this comment

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

And this will always be the contract owner only, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Now, when I think about it again, maybe this event doesn't even have to emit this information. The owner will always be an MSIG contract, probably MCMS.

event RegistryUnlockedV1(address indexed unlockedBy);
event RegistryLockedV1(address lockedBy);
event RegistryUnlockedV1(address unlockedBy);

error AddressNotAuthorized(address caller);
error CallerIsNotWorkflowOwner(address caller);
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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.
Expand All @@ -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];
Expand All @@ -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);
}
}
}
Expand All @@ -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
Expand All @@ -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);
}
}

Expand Down
Loading
Loading