-
Notifications
You must be signed in to change notification settings - Fork 308
fix(pulse-scheduler): validate atomic subscription updates using slots instead of timestamps #2590
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
fix(pulse-scheduler): validate atomic subscription updates using slots instead of timestamps #2590
Conversation
…b/pulse-scheduler/validate-update-has-matching-slots
…b/pulse-scheduler/validate-update-has-matching-slots
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
5 Skipped Deployments
|
ali-behjati
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This overall looks good to me. I like to know the code size and gas consumption changes before merging though. Another concern that I have is whether we should audit the Pyth contract now that's changed more.
Yeah good callouts. Will test both and get back to you. If we can get the Pyth contract included in the scope that would be great :) I will reach out. |
|
@ali-bahjati As for the contract size, we're juuust slim enough at 24,125 bytes :) See here Will merge, lmk if you have any concerns |
Summary
This PR fixes the "atomic" subscription update guarantee (ensuring all feeds in a subscription are updated together,) by using slots instead of timestamps for the validation.
Rationale
Use slots instead of timestamps
The main motivation for this is that timestamps (
publishTime) can be misleading for validating an "atomic" subscription update as one or more assets might have closed markets, and will retain timestamps from their last trading period.Thus, we use Pythnet slots for the atomicity validation since they are consistent across all assets regardless of market status.
Get the slots data from the Pyth parse functions
Pythnet slot number is available in the AccumulatorUpdate, but unfortunately
Pyth.parsePriceFeedUpdatesdoesn't return them, just the Price[] structs. Thus, we addparsePriceFeedUpdatesWithSlotsto the Pyth contract to expose slot information, and we consume this from the Scheduler contract.slotsarray of len(updateData) rather than len(priceFeedIds), but I chose to return parallel arrays ofPrice[] feedsanduint64[] slotsto keep things coherent for the user, especially if len(updateData) > 1.parsePriceUpdatesInternalwas already very close to the stack depth limit, and adding aslotsarray took it (quite far) over the line. Thus, this PR also contains significant refactors to that method to split it up into helper methods to reduce stack frame size. We also use structs to pass arguments instead of local variables for the same reason, since passing many arguments to functions takes up lots of stack space.Misc
Note to reviewers
I'd start in Pyth.sol first with the changes to parsePriceFeedUpdatesInternal. Then Scheduler.sol since it is downstream. The rest is somewhat mechanical fallout. Some of the motivations behind the changes are nonobvious, see below for the rationale.
How has this been tested?
Pyth.parsePriceFeedUpdatesWithSlotsbehavior