-
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
Validate answer tests and helper contract #15243
Conversation
I see you updated files related to |
AER Report: Operator UI CIaer_workflow , commit , Breaking Changes GQL Check 1. Workflow conclusion is failure:[convictional/trigger-workflow-and-wait@f69fa9e]Source of Error:
Why: The triggered workflow did not complete successfully. The status check returned a conclusion of "failure," which caused the upstream job to propagate this failure. Suggested fix: Investigate the logs of the downstream workflow (ID: 11842413061) to identify the specific cause of failure. Address the root cause in the downstream workflow to ensure it completes successfully. AER Report: CI Core ran successfully ✅ |
Static analysis results are availableHey @eduard-cl, you can view Slither reports in the job summary here or download them as artifact here. |
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.
Should this be named DualAggregatorHelper.t.sol?
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.
Good question, I was not sure about that because the other helper contracts don't include the t, an example is the OffRampHelper.sol
int192[] internal answers = [int192(10), int192(11), int192(12), int192(13), int192(14), int192(15)]; | ||
uint32[] internal observationsTimestamps = [uint32(1), uint32(6), uint32(11), uint32(16), uint32(21), uint32(26)]; | ||
uint32[] internal recordedTimestamps = [uint32(5), uint32(10), uint32(15), uint32(20), uint32(25), uint32(30)]; |
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.
These state variables should be named according to the solidity style guide, s_answers, s_observationsTImestamps, s_recordedTimestamps
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.
Do they need to be state variables?
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.
Just removed all of them and dynamically included the report values in the injectTransmissions()
function
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.
Left some comments, otherwise LGTM
Quality Gate passedIssues Measures |
_validateAnswer()
tests + move harness exposed functions to helper contract