Transmit function #15091
Transmit function #15091
Conversation
|
I see you updated files related to |
|
the code itself looks good to me, are there any failing test cases at the moment? |
AER Report: Operator UI CIaer_workflow , commit , Breaking Changes GQL Check 1. Workflow failed to complete successfully:[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" instead of "success," causing the upstream job to propagate the failure. Suggested fix: Investigate the logs of the downstream workflow (ID: 11745315311) to identify the specific reason for the failure and address the underlying issue. AER Report: CI Core ran successfully ✅ |
|
Let's also add a second entrypoint and remove the proxy as sender check in transmit |
| @@ -524,17 +546,17 @@ contract DualAggregator is OCR2Abstract, OwnerIsCreator, AggregatorV2V3Interface | |||
|
|
|||
| // decreasing loop from the latest primary round id | |||
| for (uint32 round_ = latestAggregatorRoundId; round_ > 0; --round_) { | |||
| // in case it's the latest secondary round id, return it | |||
| if (round_ == s_hotVars.latestSecondaryRoundId || latestAggregatorRoundId - round_ == s_maxSyncIterations) { | |||
There was a problem hiding this comment.
Small gas optimization: store s_maxSyncIterations in a variable above the for loop and use that variable instead.
There was a problem hiding this comment.
Also this isn't correct. If it reaches the max iterations without having found the round, we shouldn't return anything. We should revert.
There was a problem hiding this comment.
We should also add a test case that covers exceeded the max iterations
There was a problem hiding this comment.
Not sure about reverting it, this internal function is called from all the get and latest methods.
Reverting it we make them all inaccessible and we can break many client applications.
I would simply have it to stop the loop, just like we have the cutoff time or the comparison with the latestSecondaryRoundId.
There was a problem hiding this comment.
Also, I'm not sure if it's really useful, since we already have the cutoff time to stop the loop, the maxIterations should always be bigger than the cutoff time, if not we would be returning a newer price than the cutoff time.
The only two cases I see:
cutoffTime > maxIterations-> maxIterations has no effect to the loop.cutoffTime < maxIterations-> if we need the maxIterations, this means that the cutoffTime is too big and we should reduce it (also, we would be returning a newer round than the cutoff time).
There was a problem hiding this comment.
It's a valid point. The only purpose of the maxIterations variable would be so that we don't ever run into an out of gas error. As you mentioned, this would only happen if we have a very large cutoff time, and the secondary feed has been down for a long time.
But we should still have defined behaviour for those cases.
I just realized we have 2 helper functions that loop through rounds:
- _doesReportExist (called when writing)
- _getSyncPrimaryRound (called when reading)
For reading, if we reach maxIterations then returning latestSecondaryRoundId is the behaviour we want.
For writing, I believe if we reach maxIterations we should revert.
|
| // check if this round does not accomplish the cutoff time condition | ||
| if (transmission.recordedTimestamp + s_cutoffTime < block.timestamp) { | ||
| // in case it's the latest secondary round id, return it | ||
| if (round_ == s_hotVars.latestSecondaryRoundId || latestAggregatorRoundId - round_ == maxIterations) { |
There was a problem hiding this comment.
Isn't the second condition excluded by the previous if?
There was a problem hiding this comment.
You're correct, I fixed it in this PR.
| Report memory report_ = _decodeReport(report); // Decode the report | ||
|
|
||
| if (isSecondary) { | ||
| (bool exist, uint32 roundId) = _doesReportExist(report_); |
There was a problem hiding this comment.
Wouldn't it be easier and cheaper to keep s_hotVars.latestEpochAndRoundSecondary for rejecting stale reports?
There was a problem hiding this comment.
Yes, as I already see you found, we implemented this in the Check round ids PR.
| // get the latest round ids | ||
| uint32 latestAggregatorRoundId = s_hotVars.latestAggregatorRoundId; | ||
| uint32 latestSecondaryRoundId = s_hotVars.latestSecondaryRoundId; | ||
|
|
||
| // check if the message sender is the secondary proxy | ||
| if (msg.sender == s_secondaryProxy) { |
Modify the transmit function to handle secondary proxy.