[DEVSVCS-518] Workflow Registry Contract #14990
Conversation
eutopian
force-pushed
the
workflow-registry-contract-draft
branch
from
1e373da to
ea6b7ff
Compare
AER Report: CI Core ran successfully ✅AER Report: Operator UI CI ran successfully ✅ |
eutopian
force-pushed
the
workflow-registry-contract-draft
branch
from
af3a3ae to
8cc5835
Compare
|
eutopian
force-pushed
the
workflow-registry-contract-draft
branch
from
653d06d to
8f37a80
Compare
eutopian
force-pushed
the
workflow-registry-contract-draft
branch
3 times, most recently
from
3ea3eab to
40023d2
Compare
eutopian
changed the title
| @@ -132,6 +132,19 @@ let config = { | |||
| version: '0.8.19', | |||
| settings: COMPILER_SETTINGS, | |||
| }, | |||
| 'src/v0.8/workflow/dev/WorkflowRegistry.sol': { | |||
There was a problem hiding this comment.
the hardhat exclusion isn't working and we need it for the ir compile
|
The PR isn't forge formatted but also adds its code to prettierignore. This means there is no formatting which is not allowed |
eutopian
force-pushed
the
workflow-registry-contract-draft
branch
4 times, most recently
from
ed80b5c to
0ec55e3
Compare
eutopian
force-pushed
the
workflow-registry-contract-draft
branch
from
9804fe8 to
d833b24
Compare
eutopian
force-pushed
the
workflow-registry-contract-draft
branch
2 times, most recently
from
a0e852c to
903f65d
Compare
eutopian
force-pushed
the
workflow-registry-contract-draft
branch
from
903f65d to
cd0b041
Compare
|
|
||
| /// @notice Maps a combination of address and chain ID to the version number. | ||
| /// @dev This mapping allows for lookup of the version number for a given address and chain ID. | ||
| mapping(bytes32 => uint32) private s_versionNumberByAddressAndChainID; |
There was a problem hiding this comment.
nit: ideally we wanna group mappings to facilitate packing of non-256bit storage vars
| limit = MAX_PAGINATION_LIMIT; | ||
| } | ||
|
|
||
| uint32 end = (start + limit - 1 > totalVersions) ? totalVersions : start + limit - 1; |
There was a problem hiding this comment.
nit, would this be more readable:
uint32 end = start + limit - 1 > totalVersions;
if (end > totalVersions) {
end = totalVersions;
}
There was a problem hiding this comment.
What you have is the default go-minded thinking in me as well. Without considerations for readability, the ternary is supposedly "slightly" more efficient?
Readability is subjective. Maybe just add a rule for no nested ternaries period.
| // 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 | ||
| bytes32 oldSecretsHash = keccak256(abi.encodePacked(msg.sender, workflow.secretsURL)); |
There was a problem hiding this comment.
nit: abi.encodePacked is being considered for deprecation, is there a reason not to use abi.encode given you are hashing it to bytes32 anyway? Same for many other cases
There was a problem hiding this comment.
I didn't know encodePacked is being considered for depreciation, I guess it was chosen for a few reasons here
- there's multiple inputs and this is more packed/ efficient than encode.
- encode is "safer" and better because the order matters, but in most of the cases in the registry contract it's not computed directly but instead with a helper function where the two params have different types so there's no way to mix them up and in that sense the order is already pre-determined.
But I guess to your point in this version tracker contract without the helper encode itself could totally work
There was a problem hiding this comment.
But interesting.... seems like for these specific use cases of hashing addresses with something else in particular encodePacked is the best usecase ethereum/solidity#11593 (comment) kind of a bummer about the removal?
| /// new workflows for a removed DON. | ||
| /// @param donIDs The list of unique identifiers for Workflow DONs. | ||
| /// @param allowed True if they should be added to the allowlist, false to remove them. | ||
| function updateAllowedDONs(uint32[] calldata donIDs, bool allowed) external onlyOwner registryNotLocked { |
There was a problem hiding this comment.
nit: can consider 2 params uint32[] calldata donIDsToAdd, uint32[] calldata donIDsToRemove so updates can be done in single tx, ditto for funcs below
| bool sameConfigURL = Strings.equal(workflow.configURL, configURL); | ||
| bool sameSecretsURL = Strings.equal(workflow.secretsURL, secretsURL); | ||
| if (sameBinaryURL && sameConfigURL && sameSecretsURL) { | ||
| revert WorkflowContentNotUpdated(); |
There was a problem hiding this comment.
nit: does it conflict with If the value should remain unchanged, provide the same value as before.?
There was a problem hiding this comment.
Hmm I don't think so in that one of the three must change. So it's possible for 2 of the 3 to remain unchanged but if no change is the desire the user would have to provide the same value as what is already in storage instead of leaving it out of the params
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Initial dev portion of the workflow registry contract ported from dev-platform