Skip to content

Conversation

@guibescos
Copy link
Contributor

@guibescos guibescos commented Apr 3, 2024

Addresses comment about the docs from #1408

@vercel
Copy link

vercel bot commented Apr 3, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Ignored Deployments
Name Status Preview Comments Updated (UTC)
example-oracle-amm ⬜️ Ignored (Inspect) Visit Preview Apr 9, 2024 0:42am
xc-admin-frontend ⬜️ Ignored (Inspect) Visit Preview Apr 9, 2024 0:42am

@guibescos guibescos changed the title Feat/solana pusher docs(solana_sdk): Improve readme Apr 3, 2024
@guibescos guibescos changed the title docs(solana_sdk): Improve readme docs(solana_sdk): Improve docs Apr 3, 2024
@guibescos guibescos marked this pull request as ready for review April 3, 2024 16:10
/**
* Add instructions to update price feed accounts to the builder.
* Price feed accounts are a special type of price update accounts. Instead of being ephemeral accounts, they are fixed accounts for each feed id that can only be updated with a more recent price update than the one they contain.
* Use this function to update a shared price feed account with a recent price
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a 100% satisfied with this explanation

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah... I think one way to fix this is to create a richer mental model for the reader to understand how things work. I think the model is something like:

  • Pyth allows you to post any price update on-chain. This info is stored in price update accounts
  • Pyth additionally supports price feed accounts, which hold a stream of price updates that move forward in time. Programs can read a price feed account to get a recent price for a specific feed id. Price feeds are built on top of pull updates (i.e., using price update accounts).

Key thing to clarify is the relationship between price feeds and price updates. Is a feed composed of price updates? is it a sequence of updatse?

## Example use
## Price update accounts vs price feed accounts

In the pure pull model, each price update gets posted to an ephemeral account that can be closed after being consumed to reclaim rent. The SDK exposes this functionality with `addPostPriceUpdates` and `addPostPartiallyVerifiedPriceUpdates`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Start this section by explaining what you're going to explain: "Pyth can be used in two different ways on Solana. Users can either work with price updates or price feeds." You can put some of the more abstract discussion from the comment below into the README here as well.

2nd paragraph can then start with "By default, Pyth uses a pull model where" . The phrasing here suggests that the reader knows what the "pure pull model" is, which they likely don't.

Copy link
Contributor

Choose a reason for hiding this comment

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

also suggest moving the discussion of closing / rent to later in the text. The important thing here is the ephemeral accounts, so end the sentence there.

See https://www.georgegopen.com/uploads/1/0/9/0/109073507/litigation_2_stress_position.pdf

Copy link
Contributor

Choose a reason for hiding this comment

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

Also maybe for 100% clarity suggest saying explicitly that "Such accounts are called price update accounts"

* `addPostPriceUpdates` vs `addUpdatePriceFeed`:
* - `addPostPriceUpdates` is used to post price updates to ephemeral accounts. Use this to post a price update from the present or from the past for your program to consume.
* - `addUpdatePriceFeed` is used to post price updates to price feed accounts, these are fixed accounts for each feed id that can only be updated with a more recent price update than the one they contain. Their addresses can be found using `getPriceFeedAccountAddress`. Use this to update a shared price feed account with a recent price
*
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you want to move the more abstract discussion to this comment and then have these lower-level comments on the methods.

/**
* Add instructions to update price feed accounts to the builder.
* Price feed accounts are a special type of price update accounts. Instead of being ephemeral accounts, they are fixed accounts for each feed id that can only be updated with a more recent price update than the one they contain.
* Use this function to update a shared price feed account with a recent price
Copy link
Contributor

Choose a reason for hiding this comment

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

yeah... I think one way to fix this is to create a richer mental model for the reader to understand how things work. I think the model is something like:

  • Pyth allows you to post any price update on-chain. This info is stored in price update accounts
  • Pyth additionally supports price feed accounts, which hold a stream of price updates that move forward in time. Programs can read a price feed account to get a recent price for a specific feed id. Price feeds are built on top of pull updates (i.e., using price update accounts).

Key thing to clarify is the relationship between price feeds and price updates. Is a feed composed of price updates? is it a sequence of updatse?

@guibescos guibescos merged commit bb830e1 into main Apr 9, 2024
@guibescos guibescos deleted the feat/solana-pusher branch April 9, 2024 12:42
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.

3 participants