Skip to content

Commit

Permalink
address comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@eutopian
eutopian committed Nov 25, 2024
1 parent 8e9b0ee commit 176a001
Show file tree
Hide file tree
Showing 19 changed files with 100 additions and 54 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/solidity-foundry.yml
View file
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": 96.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
39 changes: 16 additions & 23 deletions contracts/src/v0.8/workflow/dev/WorkflowRegistry.sol
View file
Original file line number Diff line number Diff line change
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,31 +378,28 @@ 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, sender, donID, workflow.workflowName);
emit WorkflowDeletedV1(workflow.workflowID, msg.sender, workflow.donID, workflow.workflowName);

// Delete the workflow metadata from storage
delete s_workflows[workflowKey];
Expand Down Expand Up @@ -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
2 changes: 1 addition & 1 deletion contracts/src/v0.8/workflow/test/WorkflowRegistry/WorkflowRegistry.activateWorkflow.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ contract WorkflowRegistry_activateWorkflow is WorkflowRegistrySetup {
);

// It should emit {WorkflowActivatedV1} when the workflow is activated.
vm.expectEmit(true, true, true, true);
vm.expectEmit();
emit WorkflowRegistry.WorkflowActivatedV1(
s_validWorkflowID, s_authorizedAddress, s_allowedDonID, s_validWorkflowName
);
Expand Down
4 changes: 2 additions & 2 deletions contracts/src/v0.8/workflow/test/WorkflowRegistry/WorkflowRegistry.deleteWorkflow.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ contract WorkflowRegistry_deleteWorkflow is WorkflowRegistrySetup {
assertEq(workflow.workflowName, s_validWorkflowName);

// It should emit {WorkflowDeletedV1} when the workflow is deleted.
vm.expectEmit(true, true, true, true);
vm.expectEmit();
emit WorkflowRegistry.WorkflowDeletedV1(s_validWorkflowID, s_authorizedAddress, s_allowedDonID, s_validWorkflowName);

// Delete the workflow.
Expand All @@ -84,7 +84,7 @@ contract WorkflowRegistry_deleteWorkflow is WorkflowRegistrySetup {
_removeDONFromAllowedDONs(s_allowedDonID);

// It should emit {WorkflowDeletedV1} when the workflow is deleted.
vm.expectEmit(true, true, true, true);
vm.expectEmit();
emit WorkflowRegistry.WorkflowDeletedV1(s_validWorkflowID, s_authorizedAddress, s_allowedDonID, s_validWorkflowName);

// Delete the workflow.
Expand Down
1 change: 1 addition & 0 deletions contracts/src/v0.8/workflow/test/WorkflowRegistry/WorkflowRegistry.getAllAllowedDONs.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ contract WorkflowRegistry_getAllAllowedDONs is WorkflowRegistrySetup {
s_registry.lockRegistry();

// It should behave the same as when the registry is not locked
vm.prank(s_stranger);
uint32[] memory allowedDONs = s_registry.getAllAllowedDONs();
assertEq(allowedDONs.length, 1);
assertEq(allowedDONs[0], s_allowedDonID);
Expand Down
3 changes: 2 additions & 1 deletion .../src/v0.8/workflow/test/WorkflowRegistry/WorkflowRegistry.getAllAuthorizedAddresses.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ pragma solidity 0.8.24;

import {WorkflowRegistrySetup} from "./WorkflowRegistrySetup.t.sol";

contract WorkflowRegistrygetAllAuthorizedAddresses is WorkflowRegistrySetup {
contract WorkflowRegistry_getAllAuthorizedAddresses is WorkflowRegistrySetup {
function test_WhenTheSetOfAuthorizedAddressesIsEmpty() external {
// Remove the authorized address added in the setup
_removeAddressFromAuthorizedAddresses(s_authorizedAddress);
Expand Down Expand Up @@ -35,6 +35,7 @@ contract WorkflowRegistrygetAllAuthorizedAddresses is WorkflowRegistrySetup {
s_registry.lockRegistry();

// It should behave the same as when the registry is not locked
vm.prank(s_stranger);
address[] memory authorizedAddresses = s_registry.getAllAuthorizedAddresses();
assertEq(authorizedAddresses.length, 1);
assertEq(authorizedAddresses[0], s_authorizedAddress);
Expand Down
1 change: 1 addition & 0 deletions contracts/src/v0.8/workflow/test/WorkflowRegistry/WorkflowRegistry.getWorkflowMetadata.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ contract WorkflowRegistry_getWorkflowMetadata is WorkflowRegistrySetup {
s_registry.lockRegistry();

// It should behave the same as when the registry is not locked
vm.prank(s_stranger);
WorkflowRegistry.WorkflowMetadata memory metadata =
s_registry.getWorkflowMetadata(s_authorizedAddress, s_validWorkflowName);

Expand Down
4 changes: 2 additions & 2 deletions ...c/v0.8/workflow/test/WorkflowRegistry/WorkflowRegistry.getWorkflowMetadataListByDON.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,7 @@ import {WorkflowRegistryWithFixture} from "./WorkflowRegistryWithFixture.t.sol";

contract WorkflowRegistry_getWorkflowMetadataListByDON is WorkflowRegistryWithFixture {
function test_WhenStartIs0() external view {
WorkflowRegistry.WorkflowMetadata[] memory workflows =
s_registry.getWorkflowMetadataListByDON(s_allowedDonID, 0, 10);
WorkflowRegistry.WorkflowMetadata[] memory workflows = s_registry.getWorkflowMetadataListByDON(s_allowedDonID, 0, 0);

assertEq(workflows.length, 3);
assertEq(workflows[0].workflowName, s_workflowName1);
Expand Down Expand Up @@ -130,6 +129,7 @@ contract WorkflowRegistry_getWorkflowMetadataListByDON is WorkflowRegistryWithFi
s_registry.lockRegistry();

// It should behave the same as when the registry is not locked
vm.prank(s_stranger);
WorkflowRegistry.WorkflowMetadata[] memory workflows =
s_registry.getWorkflowMetadataListByDON(s_allowedDonID, 0, 10);

Expand Down
50 changes: 49 additions & 1 deletion ...v0.8/workflow/test/WorkflowRegistry/WorkflowRegistry.getWorkflowMetadataListByOwner.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,18 +11,27 @@ contract WorkflowRegistry_getWorkflowMetadataListByOwner is WorkflowRegistryWith

assertEq(workflows.length, 3);
assertEq(workflows[0].workflowName, s_workflowName1);
assertEq(workflows[0].owner, s_authorizedAddress);
assertEq(workflows[0].donID, s_allowedDonID);
assertTrue(workflows[0].status == WorkflowRegistry.WorkflowStatus.ACTIVE);
assertEq(workflows[0].workflowID, s_workflowID1);
assertEq(workflows[0].binaryURL, s_binaryURL1);
assertEq(workflows[0].configURL, s_configURL1);
assertEq(workflows[0].secretsURL, s_secretsURL1);

assertEq(workflows[1].workflowName, s_workflowName2);
assertEq(workflows[1].owner, s_authorizedAddress);
assertEq(workflows[1].donID, s_allowedDonID);
assertTrue(workflows[1].status == WorkflowRegistry.WorkflowStatus.ACTIVE);
assertEq(workflows[1].workflowID, s_workflowID2);
assertEq(workflows[1].binaryURL, s_binaryURL2);
assertEq(workflows[1].configURL, s_configURL2);
assertEq(workflows[1].secretsURL, s_secretsURL2);

assertEq(workflows[2].workflowName, s_workflowName3);
assertEq(workflows[2].owner, s_authorizedAddress);
assertEq(workflows[2].donID, s_allowedDonID);
assertTrue(workflows[2].status == WorkflowRegistry.WorkflowStatus.ACTIVE);
assertEq(workflows[2].workflowID, s_workflowID3);
assertEq(workflows[2].binaryURL, s_binaryURL3);
assertEq(workflows[2].configURL, s_configURL3);
Expand All @@ -35,12 +44,18 @@ contract WorkflowRegistry_getWorkflowMetadataListByOwner is WorkflowRegistryWith

assertEq(workflows.length, 2);
assertEq(workflows[0].workflowName, s_workflowName2);
assertEq(workflows[0].owner, s_authorizedAddress);
assertEq(workflows[0].donID, s_allowedDonID);
assertTrue(workflows[0].status == WorkflowRegistry.WorkflowStatus.ACTIVE);
assertEq(workflows[0].workflowID, s_workflowID2);
assertEq(workflows[0].binaryURL, s_binaryURL2);
assertEq(workflows[0].configURL, s_configURL2);
assertEq(workflows[0].secretsURL, s_secretsURL2);

assertEq(workflows[1].workflowName, s_workflowName3);
assertEq(workflows[1].owner, s_authorizedAddress);
assertEq(workflows[1].donID, s_allowedDonID);
assertTrue(workflows[1].status == WorkflowRegistry.WorkflowStatus.ACTIVE);
assertEq(workflows[1].workflowID, s_workflowID3);
assertEq(workflows[1].binaryURL, s_binaryURL3);
assertEq(workflows[1].configURL, s_configURL3);
Expand All @@ -54,12 +69,18 @@ contract WorkflowRegistry_getWorkflowMetadataListByOwner is WorkflowRegistryWith
assertEq(workflows.length, 2);
assertEq(workflows[0].workflowName, s_workflowName1);
assertEq(workflows[0].workflowID, s_workflowID1);
assertEq(workflows[0].owner, s_authorizedAddress);
assertEq(workflows[0].donID, s_allowedDonID);
assertTrue(workflows[0].status == WorkflowRegistry.WorkflowStatus.ACTIVE);
assertEq(workflows[0].binaryURL, s_binaryURL1);
assertEq(workflows[0].configURL, s_configURL1);
assertEq(workflows[0].secretsURL, s_secretsURL1);

assertEq(workflows[1].workflowName, s_workflowName2);
assertEq(workflows[1].workflowID, s_workflowID2);
assertEq(workflows[1].owner, s_authorizedAddress);
assertEq(workflows[1].donID, s_allowedDonID);
assertTrue(workflows[1].status == WorkflowRegistry.WorkflowStatus.ACTIVE);
assertEq(workflows[1].binaryURL, s_binaryURL2);
assertEq(workflows[1].configURL, s_configURL2);
assertEq(workflows[1].secretsURL, s_secretsURL2);
Expand All @@ -71,18 +92,27 @@ contract WorkflowRegistry_getWorkflowMetadataListByOwner is WorkflowRegistryWith

assertEq(workflows.length, 3);
assertEq(workflows[0].workflowName, s_workflowName1);
assertEq(workflows[0].owner, s_authorizedAddress);
assertEq(workflows[0].donID, s_allowedDonID);
assertTrue(workflows[0].status == WorkflowRegistry.WorkflowStatus.ACTIVE);
assertEq(workflows[0].workflowID, s_workflowID1);
assertEq(workflows[0].binaryURL, s_binaryURL1);
assertEq(workflows[0].configURL, s_configURL1);
assertEq(workflows[0].secretsURL, s_secretsURL1);

assertEq(workflows[1].workflowName, s_workflowName2);
assertEq(workflows[1].owner, s_authorizedAddress);
assertEq(workflows[1].donID, s_allowedDonID);
assertTrue(workflows[1].status == WorkflowRegistry.WorkflowStatus.ACTIVE);
assertEq(workflows[1].workflowID, s_workflowID2);
assertEq(workflows[1].binaryURL, s_binaryURL2);
assertEq(workflows[1].configURL, s_configURL2);
assertEq(workflows[1].secretsURL, s_secretsURL2);

assertEq(workflows[2].workflowName, s_workflowName3);
assertEq(workflows[2].owner, s_authorizedAddress);
assertEq(workflows[2].donID, s_allowedDonID);
assertTrue(workflows[2].status == WorkflowRegistry.WorkflowStatus.ACTIVE);
assertEq(workflows[2].workflowID, s_workflowID3);
assertEq(workflows[2].binaryURL, s_binaryURL3);
assertEq(workflows[2].configURL, s_configURL3);
Expand All @@ -95,18 +125,27 @@ contract WorkflowRegistry_getWorkflowMetadataListByOwner is WorkflowRegistryWith

assertEq(workflows.length, 3);
assertEq(workflows[0].workflowName, s_workflowName1);
assertEq(workflows[0].owner, s_authorizedAddress);
assertEq(workflows[0].donID, s_allowedDonID);
assertTrue(workflows[0].status == WorkflowRegistry.WorkflowStatus.ACTIVE);
assertEq(workflows[0].workflowID, s_workflowID1);
assertEq(workflows[0].binaryURL, s_binaryURL1);
assertEq(workflows[0].configURL, s_configURL1);
assertEq(workflows[0].secretsURL, s_secretsURL1);

assertEq(workflows[1].workflowName, s_workflowName2);
assertEq(workflows[1].owner, s_authorizedAddress);
assertEq(workflows[1].donID, s_allowedDonID);
assertTrue(workflows[1].status == WorkflowRegistry.WorkflowStatus.ACTIVE);
assertEq(workflows[1].workflowID, s_workflowID2);
assertEq(workflows[1].binaryURL, s_binaryURL2);
assertEq(workflows[1].configURL, s_configURL2);
assertEq(workflows[1].secretsURL, s_secretsURL2);

assertEq(workflows[2].workflowName, s_workflowName3);
assertEq(workflows[2].owner, s_authorizedAddress);
assertEq(workflows[2].donID, s_allowedDonID);
assertTrue(workflows[2].status == WorkflowRegistry.WorkflowStatus.ACTIVE);
assertEq(workflows[2].workflowID, s_workflowID3);
assertEq(workflows[2].binaryURL, s_binaryURL3);
assertEq(workflows[2].configURL, s_configURL3);
Expand All @@ -116,7 +155,6 @@ contract WorkflowRegistry_getWorkflowMetadataListByOwner is WorkflowRegistryWith
function test_WhenTheOwnerHasNoWorkflows() external view {
WorkflowRegistry.WorkflowMetadata[] memory workflows =
s_registry.getWorkflowMetadataListByOwner(s_unauthorizedAddress, 0, 10);

assertEq(workflows.length, 0);
}

Expand All @@ -133,23 +171,33 @@ contract WorkflowRegistry_getWorkflowMetadataListByOwner is WorkflowRegistryWith
s_registry.lockRegistry();

// It should behave the same as when the registry is not locked
vm.prank(s_stranger);
WorkflowRegistry.WorkflowMetadata[] memory workflows =
s_registry.getWorkflowMetadataListByOwner(s_authorizedAddress, 0, 0);

assertEq(workflows.length, 3);
assertEq(workflows[0].workflowName, s_workflowName1);
assertEq(workflows[0].owner, s_authorizedAddress);
assertEq(workflows[0].donID, s_allowedDonID);
assertTrue(workflows[0].status == WorkflowRegistry.WorkflowStatus.ACTIVE);
assertEq(workflows[0].workflowID, s_workflowID1);
assertEq(workflows[0].binaryURL, s_binaryURL1);
assertEq(workflows[0].configURL, s_configURL1);
assertEq(workflows[0].secretsURL, s_secretsURL1);

assertEq(workflows[1].workflowName, s_workflowName2);
assertEq(workflows[1].owner, s_authorizedAddress);
assertEq(workflows[1].donID, s_allowedDonID);
assertTrue(workflows[1].status == WorkflowRegistry.WorkflowStatus.ACTIVE);
assertEq(workflows[1].workflowID, s_workflowID2);
assertEq(workflows[1].binaryURL, s_binaryURL2);
assertEq(workflows[1].configURL, s_configURL2);
assertEq(workflows[1].secretsURL, s_secretsURL2);

assertEq(workflows[2].workflowName, s_workflowName3);
assertEq(workflows[2].owner, s_authorizedAddress);
assertEq(workflows[2].donID, s_allowedDonID);
assertTrue(workflows[2].status == WorkflowRegistry.WorkflowStatus.ACTIVE);
assertEq(workflows[2].workflowID, s_workflowID3);
assertEq(workflows[2].binaryURL, s_binaryURL3);
assertEq(workflows[2].configURL, s_configURL3);
Expand Down
4 changes: 2 additions & 2 deletions contracts/src/v0.8/workflow/test/WorkflowRegistry/WorkflowRegistry.lockRegistry.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import {Ownable2Step} from "../../../shared/access/Ownable2Step.sol";
import {WorkflowRegistry} from "../../dev/WorkflowRegistry.sol";
import {WorkflowRegistrySetup} from "./WorkflowRegistrySetup.t.sol";

contract WorkflowRegistrylockRegistry is WorkflowRegistrySetup {
contract WorkflowRegistry_lockRegistry is WorkflowRegistrySetup {
function test_RevertWhen_TheCallerIsNotTheContractOwner() external {
vm.expectRevert(Ownable2Step.OnlyCallableByOwner.selector);
s_registry.lockRegistry();
Expand All @@ -18,6 +18,6 @@ contract WorkflowRegistrylockRegistry is WorkflowRegistrySetup {
vm.prank(s_owner);
s_registry.lockRegistry();

assertEq(s_registry.isRegistryLocked(), true);
assertTrue(s_registry.isRegistryLocked());
}
}
Loading

0 comments on commit 176a001

Please sign in to comment.