Skip to content
This repository has been archived by the owner on Jun 30, 2022. It is now read-only.

Feat/add price slot check (In on-chain programs) #16

Merged
merged 13 commits into from
Feb 17, 2022

Conversation

ali-bahjati
Copy link
Contributor

@ali-bahjati ali-bahjati commented Feb 11, 2022

Adds get_current_price_status to Price struct which returns the current price status. If run on-chain with the current slot it checks whether price information has got stale or not. get_current_price uses this function to make the interface consistent.

Additional changes:

  • Moves test_instr for testing to common.rs (as it's used in the test for this feature) and renamed it to test_instr_exex_ok to be more clear.
  • Adds Borsh SerDe traits to PriceStatus as it is used for testing instructions.
  • Makes Ema fields public. It was required for creating testing Price object.

Copy link
Collaborator

@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.

see inline comment

src/lib.rs Outdated Show resolved Hide resolved
- Introduces another Price struct which doesn't have comps (published prices) and unused fields. The reason is that we want to mutate struct (to add the check) and Price Account Data has ~3kb so copying is expensive. This creates a significantly smaller one. This naming allow us to have least impact on existing consumers. The price account data is available as price account data if a consumer needs it.
- Also make Ema fields public
@ali-bahjati
Copy link
Contributor Author

@jayantk @Reisen

Can you review again?
I moved the logic to get_current_status function. But let's make sure to tell people to use get_current_status and get_current_price.

Didn't change the access levels of any of the fields and everything is public. I think it's good to discuss it later.

src/lib.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@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.

this change generally looks good. I had a couple comments on how to structure things so they're more general / easier for readers to understand. I think there's enough there that I'd like to look at this one more time before approving.

src/lib.rs Outdated Show resolved Hide resolved
src/instruction.rs Outdated Show resolved Hide resolved
tests/stale_price.rs Outdated Show resolved Hide resolved
tests/stale_price.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
Copy link
Collaborator

@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.

LGTM, but please do address the inline comments before merging.

README.md Outdated Show resolved Hide resolved
src/instruction.rs Show resolved Hide resolved
src/instruction.rs Outdated Show resolved Hide resolved
src/instruction.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
tests/common.rs Outdated Show resolved Hide resolved
@ali-bahjati ali-bahjati merged commit 210087a into main Feb 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants