Ethereum/parse price feed updates accumulators

[swimricky]

Summary

Add support accumulator update data in parsePriceFeeds

Changes

• Refactor PythAccumulator.sol and Pyth.sol` and add support for purely parsing
• Add test for parsePriceFeedUpdates with Accumulator update data

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Ignored Deployments

Name	Status	Preview	Comments	Updated (UTC)
example-oracle-amm	⬜️ Ignored (Inspect)			Jun 7, 2023 6:36pm
xc-admin-frontend	⬜️ Ignored (Inspect)			Jun 7, 2023 6:36pm

[jayantk]

I left you some suggestions for improvements here. I think @ali-bahjati should definitely look at this too because iirc there are weird gas efficiency hacks that he's the most knowledgable about.

[jayantk]

why do some of these functions take an offset while other ones assume you've sliced off any prefixed bytes already? I think it's better to be consistent.

[ali-behjati]

I think the code is great. It needs some improvements that Jayant has pointed out. Many of them are there because of my pervious code bad choices and I'm sorry about it. Some of them were gas optimisation considerations that we will do in a separate PR.

I think we can merge this once comments are addressed and you have added more tests.

[jayantk]

thanks this code is much clearer now. I think there's still some work to be done on the tests though -- see inline comments.

[jayantk]

hmm... so whenever you have a check like this for a start/end, you usually want one side to be >= and the other side to be < so that you can specify any arbitrary sized interval. In this case, you can't specify a zero-length interval. The typical convention is the start is inclusive and the end is exclusive.

anyway, i realize this check existed before you wrote this code, and we probably don't want to change the behavior now, so whatever. this is just an fyi.

[ali-behjati]

"you can't specify a zero-length interval" is wrong. We don't have zero-length on inclusive end too.

I decided to have inclusive end interval because it's more intuitive with the minPublishTime and maxPublishTime.

[jayantk]

I think the typical convention is >= and <, and following that will be more intuitive to most people.

[ali-behjati]

idk :D maybe you are right and it's very personal. I agree that with array ranges (slices) [start, end) is the convention. but when there is min & max, I feel [min, max] is more intuitive. On slice you say all indexes that are less than end. but for max min you say that a publish time that is minimum min and maximum max. Anyway,
i might be totally wrong.

[jayantk]

thanks this code is much clearer now. I think there's still some work to be done on the tests though -- see inline comments.

[ali-behjati]

I have two comments that are mostly my opinions and would be great to discuss them:

• It was hard for me to follow the code flow and I think we don't need this many methods.
• I think although it's easier to add code to updatePriceFeeds tests to avoid duplication but it makes tests more complex for a new test reader and makes debugging more difficult. I believe each test should be as simple as possible and test only a single scenario. I would break the tests in testParsePriceFeedWithWormholeMerkleWorks in multiple tests and also will probably create a separate file for all the parse price feeds tests.

[swimricky]

I have two comments that are mostly my opinions and would be great to discuss them:

• It was hard for me to follow the code flow and I think we don't need this many methods.
• I think although it's easier to add code to updatePriceFeeds tests to avoid duplication but it makes tests more complex for a new test reader and makes debugging more difficult. I believe each test should be as simple as possible and test only a single scenario. I would break the tests in testParsePriceFeedWithWormholeMerkleWorks in multiple tests and also will probably create a separate file for all the parse price feeds tests.

i'll clean up and reduce the number of methods in a separate PR for gas optimization

for your second point about breaking up the tests, i don't mind separating out parsing portion into their own tests. Since the setups between the parse and update versions will be largely the same i'm not too sure if the gain in simplicity will really be that significant. I think it would make sense to bundle them together in some of the tests just to highlight the differences between updatePriceFeeds and parsePriceFeedUpdates`

[jayantk]

thanks this looks good to me. I agree with @ali-bahjati that it can be a little hard to understand the tests, but let's look at ways to refactor them to simplify in a subsequent PR. This one is already quite big.

[jayantk]

comment for later: I notice that there's a code block like this that's repeated a lot, where you have some messages, then you do some work to extract ids, call parsePriceFeedUpdates, then do the assert below. You could simplify the tests by extracting this block out to a common helper function