-
Notifications
You must be signed in to change notification settings - Fork 119
New interface with time machine #230
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
Conversation
jayantk
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.
I left you a bunch of comments inline. The rust code is generally fine and the comments are minor. I am a little worried about the change to the build scripts / dockerfile though (see the inline comments). I think you're doing this in order to run the python tests, but it would be better to do this in a way that doesn't touch the production build pipeline.
program/rust/src/rust_oracle.rs
Outdated
| /// accounts[1] upgradded acount [signer writable] | ||
| /// accounts [2] system program | ||
| #[cfg(feature = "resize-account")] | ||
| pub fn update_version( |
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 name sounds like it updates the version field in the account, which it doesn't. It just resizes the account.
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.
The idea originally is that this is the generic upgrade account to the current version instruction. Happy to change the name though
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.
I think I will name it upgrade_account, so that you can pass accounts through it after any oracle upgrade
| place_holder: [u8; 1864], | ||
| } | ||
|
|
||
| impl Tracker for TimeMachineWrapper { |
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.
why do you need trait Tracker? Will there be other objects that implement the same interface? If not, could this be impl TimeMachineWrapper instead?
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.
There will be!
The individual SMA and Tick trackers in the wrapper will all be Trackers
| } | ||
| impl PriceAccountWrapper { | ||
| #[cfg(feature = "resize-account")] | ||
| pub fn add_first_price(&mut self) -> Result<(), OracleError> { |
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.
I think this function and the one below should verify that the current status == TRADING
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.
Right, I do not need to check the previous status but it is always the last with trading status, but for the current one, there's no such gurantee, thanks
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.
I made so that add_price verifies that things have been initialized in addition to that
pyth/tests/test_publish.py
Outdated
|
|
||
| def resize_account(price_acc_address): | ||
| """ | ||
| given a string with the pubkey of a price accountm it calls the resize instruction of the Oracle on it |
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.
typo price account
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.
typo
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.
still a typo here accountm
program/rust/src/rust_oracle.rs
Outdated
| c_entrypoint_wrapper(input) | ||
| let [_funding_account_info, price_account_info, _sysvar_clock] = match accounts { | ||
| [x, y, z] => Ok([x, y, z]), | ||
| _ => Err(ProgramError::InvalidArgument), |
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.
You need to fix this, it's actually more like :
let [funding_account, price_account, clock_account] = match accounts {
[x, y,z ] => Ok([x, y, z]),
[x, y,_ , z ] => Ok([x, y, z]),
_ => Err(ProgramError::InvalidArgument),
}?;
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.
Since this is a thin wrapper around c_entrypoint_wrapper, you want to avoid writing code that could accidentally change the behavior. The best way to have approached this problem, imo, would have been something like:
let c_ret_value = c_entrypoint_wrapper(input)?;
let price_account = accounts.get(2)?;
let account_len = ...
match {
... => price_account.add_price_to_time_machine();
}
If you structure your code that way, all of your additions happen after c_entrypoint_wrapper, which makes it hard for future edits to accidentally forget to call that function.
pyth/tests/test_publish.py
Outdated
|
|
||
| def resize_account(price_acc_address): | ||
| """ | ||
| given a string with the pubkey of a price accountm it calls the resize instruction of the Oracle on it |
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.
typo
|
|
||
| impl PythAccount for PriceAccountWrapper { | ||
| const ACCOUNT_TYPE: u32 = PC_ACCTYPE_PRICE; | ||
| const INITIAL_SIZE: u32 = size_of::<PriceAccountWrapper>() as u32; |
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.
Make this consistent with PythAccount for pc_price_t
jayantk
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.
I pretty much had the same comments as @guibescos .
program/rust/src/rust_oracle.rs
Outdated
| c_entrypoint_wrapper(input) | ||
| let [_funding_account_info, price_account_info, _sysvar_clock] = match accounts { | ||
| [x, y, z] => Ok([x, y, z]), | ||
| _ => Err(ProgramError::InvalidArgument), |
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.
Since this is a thin wrapper around c_entrypoint_wrapper, you want to avoid writing code that could accidentally change the behavior. The best way to have approached this problem, imo, would have been something like:
let c_ret_value = c_entrypoint_wrapper(input)?;
let price_account = accounts.get(2)?;
let account_len = ...
match {
... => price_account.add_price_to_time_machine();
}
If you structure your code that way, all of your additions happen after c_entrypoint_wrapper, which makes it hard for future edits to accidentally forget to call that function.
program/rust/src/rust_oracle.rs
Outdated
| Ok(()) | ||
| } | ||
|
|
||
| /// resizes a price accpunt so that it fits the Time Machine |
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.
typo: account
pyth/tests/test_publish.py
Outdated
|
|
||
| def resize_account(price_acc_address): | ||
| """ | ||
| given a string with the pubkey of a price accountm it calls the resize instruction of the Oracle on it |
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.
still a typo here accountm
guibescos
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.
Good to go
This PR implements the resize instruction, as well as the new update price logic, without implementing the TwapTracker (which is currently left as a byte array with no_op operations).
I would appreciate feedback on:
1- the interface between rust_oracle.rs and the time_machine
2- The way I compined all the into in price_t into one struct
3- The way I dealt with integer conversion
4- The resize instruction test. (Assuming it is combined with unit_tests for add_first_price)