Skip to content
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.

Sign up for GitHub

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

Check round ids #15183

Merged
merged 8 commits into from
Nov 12, 2024
Merged

Check round ids #15183

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d7c6f01
check rounds
eduard-cl Nov 11, 2024
b67bcdc
forge fmt
eduard-cl Nov 11, 2024
cc60767
modify transmit function
eduard-cl Nov 11, 2024
037dd75
Add tests
Oozyx Nov 11, 2024
3067d87
Merge pull request #15193 from smartcontractkit/feeds-project-additio…
eduard-cl Nov 12, 2024
ba122ad
transmit function improved
eduard-cl Nov 12, 2024
674785d
improve transmit logic + add 2 tests
eduard-cl Nov 12, 2024
c112e39
add comments + verify signatures early
eduard-cl Nov 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 25 additions & 18 deletions contracts/src/v0.8/feeds/DualAggregator.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -500,8 +500,8 @@ contract DualAggregator is OCR2Abstract, OwnerIsCreator, AggregatorV2V3Interface
// max iterations the secondary proxy will be able to loop to sync with the primary rounds
uint32 internal s_maxSyncIterations;

// wether if the primary latest report has to be locked or not
bool internal s_primaryLocked;
// wether if the latest report was secondary or not
bool internal s_latestSecondary;

/**
* @notice indicates that a new report arrived from the secondary feed and the round id was updated
Expand Down Expand Up @@ -639,7 +639,7 @@ contract DualAggregator is OCR2Abstract, OwnerIsCreator, AggregatorV2V3Interface
// get the transmission
transmission = s_transmissions[latestAggregatorRoundId];
// in case the transmission was sent in this same block only by the secondary proxy, return the previous round id
if (s_primaryLocked && transmission.recordedTimestamp == block.timestamp) {
if (s_latestSecondary && transmission.recordedTimestamp == block.timestamp) {
return latestAggregatorRoundId - 1;
}
}
Expand Down Expand Up @@ -804,37 +804,46 @@ contract DualAggregator is OCR2Abstract, OwnerIsCreator, AggregatorV2V3Interface
// Validate the report data
_validateReport(reportContext, report, rs, ss);

// Verify signatures attached to report
_verifySignatures(reportContext, report, rs, ss, rawVs);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why it got moved? I have to be missing something, but isn't the main gas saving due to skipping this step if the report is already recorded?


Report memory report_ = _decodeReport(report); // Decode the report

if (isSecondary) {
(bool exist, uint32 roundId) = _doesReportExist(report_);
// In case the report exists, copy the round id and pay the transmitter
if (exist) {
// In case the round has already been processed by the secondary feed
if (s_hotVars.latestSecondaryRoundId >= roundId) {
revert StaleReport();
}

s_hotVars.latestSecondaryRoundId = roundId;
emit SecondaryRoundIdUpdated(roundId);

_payTransmitter(s_hotVars, report_.juelsPerFeeCoin, uint32(initialGas), msg.sender);
Oozyx marked this conversation as resolved.
Show resolved Hide resolved
return;
}
// In case the report doesn't exist, lock the primary feed
s_primaryLocked = true;
} else {
Copy link

@Oozyx Oozyx Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: I think we could keep this else and use it for the case where the primary feed deals with receiving an existing, but latest report.

if (epochAndRound == s_hotVars.latestEpochAndRound && s_primaryLocked))

I see this if, else block as where the logic is evaluated for all edge cases where an existing report is sent to either feed. Early returns would happen here as well.

The rest of the function would be to handle and record new reports.

This is very subjective so feel free to disagree.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you develop more this idea? Not sure I'm following how it should be implemented.

// In case the sender is the primary proxy, unlock the latest report
s_primaryLocked = false;
}

// Report epoch and round
uint40 epochAndRound = uint40(uint256(reportContext[1]));

if (epochAndRound < s_hotVars.latestEpochAndRound) {
revert StaleReport();
}
// Only skip the report transmission in case the epochAndRound is equal to the latestEpochAndRound
// and the latest sender was the secondary feed
if (epochAndRound != s_hotVars.latestEpochAndRound || !s_latestSecondary) {
// In case the epochAndRound is lower or equal than the latestEpochAndRound, it's a stale report
// because it's older or has already been transmitted
if (epochAndRound <= s_hotVars.latestEpochAndRound) {
revert StaleReport();
}

// Verify signatures attached to report
_verifySignatures(reportContext, report, rs, ss, rawVs);
_report(s_hotVars, reportContext[0], epochAndRound, report_, isSecondary);
}

int192 juelsPerFeeCoin = _report(s_hotVars, reportContext[0], epochAndRound, report_, isSecondary);
_payTransmitter(s_hotVars, juelsPerFeeCoin, uint32(initialGas), msg.sender);
// Store if the latest report was secondary or not
s_latestSecondary = isSecondary;
_payTransmitter(s_hotVars, report_.juelsPerFeeCoin, uint32(initialGas), msg.sender);
}

// helper function to validate the report data
Expand Down Expand Up @@ -953,7 +962,7 @@ contract DualAggregator is OCR2Abstract, OwnerIsCreator, AggregatorV2V3Interface
uint40 epochAndRound,
Report memory report,
bool isSecondary
) internal returns (int192 juelsPerFeeCoin) {
) internal {
if (report.observations.length > MAX_NUM_ORACLES) revert NumObservationsOutOfBounds();
// Offchain logic ensures that a quorum of oracles is operating on a matching set of at least
// 2f+1 observations. By assumption, up to f of those can be faulty, which includes being
Expand Down Expand Up @@ -1003,8 +1012,6 @@ contract DualAggregator is OCR2Abstract, OwnerIsCreator, AggregatorV2V3Interface
emit AnswerUpdated(median, hotVars.latestAggregatorRoundId, block.timestamp);

_validateAnswer(hotVars.latestAggregatorRoundId, median);

return report.juelsPerFeeCoin;
}

/**
Expand Down
Loading
Loading