refactor: Optional storage flag when parsing price feed updates #2727
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
7 Skipped Deployments
|
ayushboss
changed the title
There was a problem hiding this comment.
Thanks! Couple notes
- Can we add a test case that asserts that when the param is enabled it actually updates the underlying public feed, and the negative case as well?
- Can we add a test to the gas benchmark to check how expensive parsing + storing is?
- Do the updated contracts fit within size limits? (
forge build --sizes)
| function parsePriceFeedUpdatesWithConfig( | ||
| bytes[] calldata updateData, | ||
| bytes32[] calldata priceIds, | ||
| uint64 minPublishTime, | ||
| uint64 maxPublishTime, | ||
| bool unique | ||
| uint64 minAllowedPublishTime, | ||
| uint64 maxAllowedPublishTime, | ||
| bool checkUniqueness, | ||
| bool checkUpdateDataIsMinimal, | ||
| bool storeUpdatesIfFresh | ||
| ) | ||
| internal | ||
| returns (PythStructs.PriceFeed[] memory feeds, uint64[] memory slots) | ||
| public | ||
| payable | ||
| returns ( | ||
| PythStructs.PriceFeed[] memory feeds, | ||
| uint64[] memory slots | ||
| ) |
There was a problem hiding this comment.
IMO this is a little confusing as a public API since it exposes lots of options (prone to misconfiguration) and also returns data most people won't need (slots). Might be better to keep this as a private method and expose public APIs targeted at specific use cases. The only thing using storeUpdatesIfFresh would be Pulse via parsePriceFeedUpdatesWithSlotsStrict, so we could expose the storeUpdatesIfFresh param there. Curious on your thoughts on this @ali-bahjati .
There was a problem hiding this comment.
Hmmm I think the practice I've seen elsewhere is a combination of both. You'd keep easy to understand shortcuts for people but one that is not often used (likely we only use it ourselves) because at the same time making API for every combo is not good (it comes more as confusing to people).
There was a problem hiding this comment.
yeah it makes sense. lets make sure to include a note in the natspec doc that it's recommended in most cases to use the standard parsePriceFeedUpdates or parsePriceFeedUpdatesStrict methods.
There was a problem hiding this comment.
nice! couple small things, please address before merging. also looks like you may need to run the formatter to get CI to pass (pre-commit should help take care of this automatically going forward, see .pre-commit-config.yaml)
|
|
||
| // Check all price feeds were found | ||
| for (uint k = 0; k < priceIds.length; k++) { | ||
| for (uint k = 0; k < priceIds.length; ++k) { |
There was a problem hiding this comment.
how is preincrement different here?
| // Should revert in strict mode | ||
| vm.expectRevert(PythErrors.InvalidArgument.selector); | ||
| pyth.parsePriceFeedUpdatesWithSlotsStrict{value: updateFee}( | ||
| pyth.parsePriceFeedUpdatesWithConfig{value: updateFee}( |
There was a problem hiding this comment.
can we update the names of these tests please
There was a problem hiding this comment.
let's bump the version() to 1.4.5-alpha.1
| override | ||
| returns (PythStructs.PriceFeed[] memory priceFeeds); | ||
|
|
||
| function parsePriceFeedUpdatesWithSlotsStrict( |
There was a problem hiding this comment.
did we squeeze within the size limit with this? :)
…th config in test functions
Summary
I updated the parsePriceFeedUpdatesInternal function to be more flexible and take an additional flag indicating whether or not parsed prices need to be stored.
Rationale
Recent changes added the parsePriceFeedUpdatesInternal function to prevent invocations of that function from updating storage. An additional flag allows us to be flexible with use cases that may need to update storage while performing parsing.
How has this been tested?
Tested using existing tests. Additionally, I added tests to verify the new parse function both when storage is enabled and disabled, checking that the parsed price signatures are found in the former case, and making sure that the price signatures are not updated in the latter. Also added gas benchmark tests.