-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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.
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
[KS-241] Update metadata passed to Forwarder and Receiver #13389
Conversation
bc04c40
to
09db180
Compare
// workflow_name // offset 64, size 10 | ||
// workflow_owner // offset 74, size 20 | ||
// report_name // offset 94, size 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is 32 bytes total. You could return just the 32 bytes and do an equality comparison (even a hash is not needed).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be tough when we allow multiple owners and/or names.
09db180
to
bdcc21a
Compare
bdcc21a
to
cbdaca0
Compare
d528899
to
6018263
Compare
6018263
to
6516bfb
Compare
1e33a20
to
82e99fc
Compare
a9fd3af
to
3ce2f6d
Compare
|
||
// f can never be 0, so this means the config doesn't actually exist | ||
if (s_configs[donId].f == 0) revert InvalidDonId(donId); | ||
|
||
bytes32 reportId = _reportId(receiverAddress, workflowExecutionId); | ||
if (s_reports[reportId].transmitter != address(0)) revert ReportAlreadyProcessed(reportId); | ||
bytes32 combinedId = _combinedId(receiverAddress, workflowExecutionId, reportId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent naming
bytes32 combinedId = _combinedId(receiverAddress, workflowExecutionId, reportId); | |
bytes32 messageId = _combinedId(receiverAddress, workflowExecutionId, reportId); |
I would prefer transmissionId. We can fix in a follow-up.
// workflow_owner // offset 119, size 20 | ||
// report_name // offset 139, size 2 | ||
if (uint8(rawReport[0]) != 1) { | ||
revert InvalidVersion(uint8(rawReport[0])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed, we don't need this validation today. We can fix it in a follow-up.
bytes10[] internal s_allowedWorkflowNamesList; | ||
mapping(bytes10 => bool) internal s_allowedWorkflowNames; | ||
|
||
function setConfig( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not efficient (I understand it is just an example). We can fix it in a follow-up gas optimization PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I will address polish comments in a follow-up PR.
No description provided.