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

Support aggregation in validator #410

Merged
merged 21 commits into from
Jul 24, 2024
Merged

Support aggregation in validator #410

merged 21 commits into from
Jul 24, 2024

Conversation

Riateche
Copy link
Contributor

@Riateche Riateche commented Jul 9, 2024

  • Change dependency versions to make the crate compatible with pythnet validator
  • Add flags to price account to track transition from v1 to v2
  • Implement transition between v1 and v2 by calling add_publisher with a special pubkey
  • Skip aggregation and message buffer update in the oracle program if v2 aggregation is enabled
  • Clear message buffers after transition to v2
  • Expose aggregation as a library function to be used in the validator

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.

left a few minor comments and one maybe significant question -- see inline

program/rust/src/processor/add_publisher.rs Outdated Show resolved Hide resolved
program/rust/src/validator.rs Show resolved Hide resolved
program/rust/src/validator.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@ali-bahjati ali-bahjati left a comment

Choose a reason for hiding this comment

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

I think as Jayant said, would be nice to test the validator update code path too.

program/rust/src/processor/add_publisher.rs Outdated Show resolved Hide resolved
program/rust/src/validator.rs Outdated Show resolved Hide resolved
@guibescos
Copy link
Contributor

guibescos commented Jul 13, 2024

I think there might be an edgecase where the price stops aggregating (both v1 and v2).
In a given slot:

  • upd_price : aggregation is unsuccessful therefore message_sent_ == 1
  • flags gets activated
  • aggregation v2 fails with V1AggregationMode since PriceAccountFlags::MESSAGE_BUFFER_CLEARED is false

Next slot:

  • upd_price no longer triggers an aggregation because PriceAccountFlags::ACCUMULATOR_V2 is true
  • aggregation v2 also fails since PriceAccountFlags::MESSAGE_BUFFER_CLEARED is false

And so on...
let me know if you agree

@Riateche
Copy link
Contributor Author

I think there might be an edgecase where the price stops aggregating (both v1 and v2).

Good catch! I've changed the code so that the MESSAGE_BUFFER_CLEARED is set regardless of the message_sent_ flag. I think this should fix the issue.

@Riateche Riateche force-pushed the accumulator-v2 branch 4 times, most recently from 9065d7b to 216eee6 Compare July 19, 2024 10:56
update_clock_slot(&mut clock_account, 2);

validator::aggregate_price(2, 102, price_account.key, *price_account.data.borrow_mut())
.unwrap();
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: can we once verify the output of this as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added some output checks.

ali-bahjati
ali-bahjati previously approved these changes Jul 23, 2024
Copy link
Collaborator

@ali-bahjati ali-bahjati left a comment

Choose a reason for hiding this comment

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

LGTM!

jayantk
jayantk previously approved these changes Jul 23, 2024
]
};

// Append a TWAP message if available
Copy link
Contributor

Choose a reason for hiding this comment

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

this comment seems like it doesn't apply to anything

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like it's no longer relevant. Removed.

@Riateche Riateche dismissed stale reviews from jayantk and ali-bahjati via 1a13ccc July 24, 2024 15:02
ali-bahjati
ali-bahjati previously approved these changes Jul 24, 2024
@Riateche Riateche merged commit a278681 into main Jul 24, 2024
3 checks passed
@Riateche Riateche deleted the accumulator-v2 branch July 24, 2024 16:04
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.

5 participants