-
Notifications
You must be signed in to change notification settings - Fork 185
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
Migrate developer portal docs #427
Conversation
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.
Haven't looked at the Legacy SDK
section yet. If you can confirm that is jus copy-pasta I can skip over it in the review
|
||
A tick-spacing of 5 means that liquidity can be deposited into tick-index that are a multiple of 5. (ex. [...-10, -5, 0, 5, 10...]). | ||
|
||
Whirlpool supports a tick-spacing between 1 and 256, preferably a value that is a power of 2 for better compute-budget optimization. |
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 is not enforced by the contract iirc. Might consider removing this
A low tick-spacing pool undergoing a massive price movement may require multiple swap instructions to complete the price movement. Therefore, more volatile pairs that often has large price swings should look at higher tick-spacing to mitigate this pain point for their pool users. | ||
|
||
### 3. Account rent cost for users | ||
On Solana, accounting information requires account space. The more information a program hosts, the more rent is required to store. The larger the tick-spacing, the less ticks it needs for the program to keep track of liquidity across a set price range. Subsequently, the less money it takes to rent space on Solana. |
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.
"accounting information"? Maybe something like "On Solana, storing data on-chain requires account space" or idk that sounds weird too because it is saying the same thing.
To me, "accounting information" sounds like some bookkeeping stuff or something
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.
Also on Solana
maybe we can just change to on chain
or something since we are not limited to solana anymore
- Account Size - 10kb | ||
|
||
## Usage in Whirlpool Program Instructions | ||
When you interact with ticks on Whirlpool instructions, often you will need to derive the correct tick-array so the program can get access to the designated tick object. |
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.
What is the purpose of this paragraph? Feels like it is missing something. Maybe the explanation on how to derive said tick-array?
i.e. I how do I know which tick-array a certain tick index is in. But then again that is also explained already in the pragraph above
When you interact with ticks on Whirlpool instructions, often you will need to derive the correct tick-array so the program can get access to the designated tick object. | ||
|
||
### Open Position | ||
A position opening up in a brand new tick/price area would need to initialize the tick-array prior to creating the position. This means the user who invoke that position would have to pay for the rent-exempt cost (tick-array accounts are 10kb). |
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 reads a little bit weird.
When a position is opened in a brand new tick/price area...
This means the user who opened that position would...
### Open Position | ||
A position opening up in a brand new tick/price area would need to initialize the tick-array prior to creating the position. This means the user who invoke that position would have to pay for the rent-exempt cost (tick-array accounts are 10kb). | ||
|
||
Whirlpool owners can consider preemptively initializing tick-array ranges to avoid user surprises. Once the tick-array account is setup, it will never have to be reinitialized. |
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.
Maybe also add that it cannot be closed along with that it does not need to be reinitialized
@@ -0,0 +1,21 @@ | |||
import DocCard from '@theme/DocCard'; | |||
|
|||
# A Note for Rust Developers |
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.
Is this page still necessary since we have rust SDKs now? Guess this is fine for now but can you create a ticket for this?
--- | ||
sidebar-label: API | ||
--- | ||
import DocCard from '@theme/DocCard'; |
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.
Same as with the Rust SDK section. I guess this is fine for now but we should probably make this it's own section in the docs with public api v2? Can you create a ticket for this?
@@ -0,0 +1,28 @@ | |||
import DocCard from '@theme/DocCard'; | |||
|
|||
# Interact via CPI |
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.
We should probably remove this (or add it to Legacy SDK) in favor of orca_whirlpools_client
from our rust sdk. Ticket?
@@ -38,6 +41,16 @@ export default { | |||
], | |||
], | |||
|
|||
stylesheets: [ | |||
{ | |||
href: 'https://cdn.jsdelivr.net/npm/[email protected]/dist/katex.min.css', |
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.
Do we want to have a cdn link? Is it also in the rehype-katex
package? If so we can reference that in node_modules
. I see it has a checksum which would prevent any remote modification I guess so not strictly necessary
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 can't seem to find it. I could copy the css code in docs/whirlpool/static/katex.min.css
. What do you think?
docs/whirlpool/package.json
Outdated
@@ -14,7 +14,9 @@ | |||
"clsx": "^2.1.1", | |||
"prism-react-renderer": "^2.4.0", | |||
"react": "^18.3.1", | |||
"react-dom": "^18.3.1" | |||
"react-dom": "^18.3.1", | |||
"rehype-katex": "7", |
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.
Better to specify full version. e.g. ^7.x.x
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.
LGTM 🚀
|
||
> ℹ️ Now Orca UI doesn't show whirlpools with TokenExtensions, so it is "contract level" support only at the moment. | ||
The only exception is PYUSD, which is supported in the UI. | ||
On May 28th, Whirlpool program has been upgraded to support TokenExtensions (aka Token-2022 program). |
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.
Maybe it still needs the year (2024). Same goes for the InterestBearing
docs/whirlpool/package.json
Outdated
@@ -15,8 +15,8 @@ | |||
"prism-react-renderer": "^2.4.0", | |||
"react": "^18.3.1", | |||
"react-dom": "^18.3.1", | |||
"rehype-katex": "7", | |||
"remark-math": "6" | |||
"rehype-katex": "7.0.1", |
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 would do "^7.0.1" instead of just "7.0.1". I believe it signifies Any version that is backwards-compatible
with this version
Co-authored-by: calintje <[email protected]>
…orca-so/whirlpools into calintje/migrate-developer-docs
Sorry I know
I was a bit confused this part: I think the description of rent in this section is too early for beginners. The description of the TE based position NFT that rent comes back at this stage may make them confused. This is because Whirlpool and TickArray cannot be closed and they require more rent. |
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.
Thanks for the migration!
### Swap | ||
Swap users will have to provide the series of tick-arrays that the swap will traverse across. | ||
|
||
The first tick-array in the sequence typically houses the Whirlpool's current tick index, though this is not always required. In some cases, such as with the new swap instruction, users can pass in up to six tick-arrays, with only three arrays being crossed during a swap. For example, the Whirlpools SDK preemptively provides the tick-array containing the current price, along with two arrays below and two above it, ensuring adequate coverage for different price ranges. |
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.
nit: How about moving In some cases, ... ensuring adequate coverage for different price ranges.
to after The second and third tick ... tick-array public key.
.
In this sentence, I think we explain about "first tick-array". So current description flow looks the following to me.
- first tick array
- Sparse swap improvement
- second and thirt tick array
- Position tokens can be freely transferred. Whoever holds the token in their wallet has the authority to manage the position account, allowing them to increase or decrease liquidity, harvest fees, and close the position. | ||
|
||
## NFT Metadata | ||
Whirlpools utilizes the Token2022 program for position NFTs, leveraging the MetadataPointer and TokenMetadata extensions to eliminate the need for Metaplex metadata accounts and associated costs. Advanced users can also choose to exclude metadata entirely. |
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 to eliminate ... associated costs
part can be replaced with to make all rent refundable
. 🙂
non-refundable rent is really pain for LPer 😢
@@ -32,7 +32,7 @@ Initialize the project as a TypeScript project: | |||
npx tsc --init | |||
``` | |||
|
|||
## 3. Wallet Creation | |||
## 2. Wallet Creation | |||
|
|||
You can create a wallet using `generateKeyPairSigner()` from the Solana SDK. |
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.
hmm, how about generating wallet.json and reading it.
This is why this wallet is not persistent and will be lost when the script completed.
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 initially used wallet.json
, but after review, switched to this simpler approach to keep the code concise. The conclusion was that persistent storage fits better in example projects, where configuration can be fully demonstrated. Let me know if you'd like changes—happy to adjust!
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.
Updated with an example using createKeyPairSignerFromBytes()
``` | ||
|
||
```tsx | ||
const connection = new Connection(url, "singleGossip"); |
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.
nit: commitment level "confirmed"
Migrate developer portal docs