Skip to content

[TWAP] Implement new twap #359

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

Merged
merged 31 commits into from
May 1, 2023
Merged

[TWAP] Implement new twap #359

merged 31 commits into from
May 1, 2023

Conversation

guibescos
Copy link
Contributor

No description provided.

@guibescos guibescos changed the base branch from main to revisit-twap April 26, 2023 22:09
@guibescos guibescos changed the title Revisit twap2 [TWAP] Implement new twap Apr 26, 2023
Base automatically changed from revisit-twap to main April 27, 2023 17:01
@guibescos guibescos marked this pull request as ready for review April 27, 2023 17:38
Copy link
Contributor

@jayantk jayantk left a comment

Choose a reason for hiding this comment

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

awesome. I have a couple asks for this one, so would like to see this again after some updates.

#[cfg(test)]
impl PriceAccountNew {
/// This function gets triggered when there's a succesful aggregation and updates the cumulative sums
pub fn update_price_cumulative(&mut self) {
Copy link
Contributor

Choose a reason for hiding this comment

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

aren't there some assumptions about trading and stuff that need to be satisfied in order to call this function?

IMO this function does so little it would be better to inline the call to update in the calling code (which also guarantees the assumptions are satisfied)

Copy link
Contributor Author

@guibescos guibescos Apr 28, 2023

Choose a reason for hiding this comment

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

The reason it's not inlined is some rust borrower annoyance that makes it not very clean.
I added a check that status == TRADING whenever this function gets called.

@@ -2,6 +2,7 @@ name: Docker

on:
push:
branches: [ main ]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Drive-by reduce CI load

@@ -200,7 +201,7 @@ static_assert( sizeof( pc_price_t ) == 3312, "" );

// This constant needs to be an upper bound of the price account size, it is used within pythd for ztsd.
// It is set tighly to the current price account + 96 component prices + 48 bytes for cumulative sums
const uint64_t PRICE_ACCOUNT_SIZE = 3312 + 96 * sizeof( pc_price_comp_t) + 48;
const uint64_t ZSTD_UPPER_BOUND = 3312 + 96 * sizeof( pc_price_comp_t) + 48;
Copy link
Contributor Author

@guibescos guibescos Apr 28, 2023

Choose a reason for hiding this comment

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

This gets renamed for clarity

ORACLE_SIZE=$(wc -c ./target/deploy/pyth_oracle.so | awk '{print $1}')
if [ $ORACLE_SIZE -lt 81760 ]
if [ $ORACLE_SIZE -lt 88429 ]
Copy link
Contributor Author

@guibescos guibescos Apr 28, 2023

Choose a reason for hiding this comment

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

I think with the c flags we can stay under this with the 3 upcoming changes : twap + resize + message-buffer

jayantk
jayantk previously approved these changes Apr 28, 2023
Copy link
Contributor

@jayantk jayantk left a comment

Choose a reason for hiding this comment

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

awesome. i think there's a minor issue with the header size still, but should be an easy fix.


impl PythAccount for PriceAccountV2 {
const ACCOUNT_TYPE: u32 = PC_ACCTYPE_PRICE;
/// Equal to the offset of `comp_` in `PriceAccount`, see the trait comment for more detail
Copy link
Contributor

Choose a reason for hiding this comment

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

comment is wrong now

Copy link
Contributor

Choose a reason for hiding this comment

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

hm i also just noticed that add_publisher sets the header.size_ field based on the number of populated price components. This field isn't actually read anywhere as far as i know (definitely not used on-chain) but we should fix that too. I think we could just set it to the number of bytes in the account and it'll be fine.

jayantk
jayantk previously approved these changes Apr 28, 2023
@guibescos
Copy link
Contributor Author

This is now blocked by #361

@guibescos guibescos merged commit 96da017 into main May 1, 2023
@guibescos guibescos deleted the revisit-twap2 branch May 1, 2023 21:12
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.

2 participants