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.

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

Add price feed rounds #64

Merged
merged 11 commits into from
Sep 12, 2024
Merged

Add price feed rounds #64

merged 11 commits into from
Sep 12, 2024

Conversation

iFrostizz
Copy link
Contributor

@iFrostizz iFrostizz commented Aug 27, 2024

Closes #63

Add rounds to have historical rounds data like getRoundData and other missing methods.

  • Make sure that all the relevant public functions at the AggregatorV3 level are included
  • Check the equivalence of function signatures

Only the getRoundData has been added so that the PriceFeedImporter contract implements the AggregatorV3Interface. It is a good security practice for callers because it gives by default the answer as well as the update time of the answer which can be checked to judge staleness and fallback on other methods if this one is not satisfying enough. Methods such as latestTimestamp or latestAnswer are deprecated.
The method latestRound to get the latest roundID though may be useful (methods are at https://github.com/smartcontractkit/libocr/blob/ae747ca5b81236ffdbf1714318c652e923a5ff4d/contract/OffchainAggregator.sol#L721-L785) and in this case we may want to still implement those for legacy purposes ?

@iFrostizz iFrostizz self-assigned this Aug 27, 2024
@iFrostizz iFrostizz marked this pull request as draft August 27, 2024 19:00
@iFrostizz iFrostizz marked this pull request as ready for review September 2, 2024 13:34
@@ -12,7 +12,6 @@ import {RLPReader} from "@solidity-merkle-trees/trie/ethereum/RLPReader.sol";
* THIS IS AN EXAMPLE LIBRARY THAT USES UN-AUDITED CODE.
* DO NOT USE THIS CODE IN PRODUCTION.
*/

Copy link
Contributor Author

@iFrostizz iFrostizz Sep 3, 2024

Choose a reason for hiding this comment

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

This happened when formatting

Comment on lines +113 to +115
if (roundID <= latestRoundID && latestRoundID != 0) {
revert("roundID should be monotonically increasing");
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Events should be published monotonically increasingly. This is not true apparently on startup because the blocks catch-up operation is done asynchronously to the tip following one, thanks @cam-schultz !
We may want to modify this logic because of the catch-up so that it can accept any height, but not allow overwrites.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An issue arising from that is that there might be some missing rounds although the last one is saved. Some smart contracts relying on this information to calculate i.e. the median price may get surprised by this behavior.
If we assume that any gap can only happen at the startup and that it would be filled quickly, we could save them in a "buffer" and only respond with the getRoundData method if none of the previous round is missing.

@iFrostizz iFrostizz merged commit 0e9a294 into main Sep 12, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add getRoundData(uint80 _roundId) to the price feed replica
1 participant