Rust workspace changes (formerly anchor-0.30 but reverted) #174
Conversation
# Conflicts: # .github/actions/anchor/action.yml # .github/workflows/publish.yml # Anchor.toml # README.md # legacy-sdk/scripts/tsconfig.json # legacy-sdk/whirlpool/package.json # package.json # yarn.lock
# Conflicts: # package.json # yarn.lock
|
Sidenote: don't think this would have to be deployed immediately since the program instructions themselves don't change. This is only needed for:
|
There was a problem hiding this comment.
Almost LGTM.
One point is rent: Sysvar<'info, Rent> part.
I think it is nice to keep it unchanged for the following reasons.
- That the name of the account is rent.
- That we want to avoid the fact that an arbitrary address is passed (safer & indexer friendly)
- The transaction size of the instruction receiving "rent" is not a major problem, imo
- That the v2 instruction can solve the open_position problem (v2 for open_position is coming)
- ALT can be used.
WDYT ?
Anchor.toml
Outdated
| # So we need to use test.genesis config. | ||
| # [[test.validator.clone]] | ||
| # address = "metaqbxxUerdq28cj1RbAWkYQm3ybzjb6a8bt518x1s" | ||
|
|
There was a problem hiding this comment.
hmm, how about using [[test.validator.clone]] again and moreve external_program/mpl_token_metadata.20240214.so, or keep this comment to explain why we stopped to use [[test.validator.clone]]
Metaplex has had breaking changes in the past, so it is somewhat safer to use the latest binaries.
They have removed the old instructions, that Whirlpool depended on, to introduce fees.
Anchor 0.30.1 fixed this issue: coral-xyz/anchor#3010
legacy-sdk/whirlpool/README.md
Outdated
| @@ -16,7 +16,7 @@ In your package, run: | |||
| ``` | |||
| yarn add "@orca-so/whirlpools-sdk" | |||
| yarn add "@orca-so/common-sdk" | |||
| yarn add "@coral-xyz/anchor@0.29.0" | |||
| yarn add "@coral-xyz/anchor@0.30.0" | |||
There was a problem hiding this comment.
Do we use 0.30.0 ? (not 0.30.1 ?) on client-side.
README.md
Outdated
| - Anchor 0.29.0 | ||
| - Solana 1.17.22 | ||
| - Anchor v0.30 | ||
| - Solana v1.18 |
There was a problem hiding this comment.
How about v0.30.1 and v1.18.21. (as officially tested version)
programs/whirlpool/Cargo.toml
Outdated
| spl-token = { version = "=4.0.3", features = ["no-entrypoint"] } | ||
| spl-transfer-hook-interface = { version = "=0.6.5" } | ||
| solana-program = { version = "=1.18.21" } | ||
| thiserror = { version = "=1.0.63" } | ||
| uint = { version = "=0.9.5", default-features = false } | ||
| arrayref = { version = "=0.3.8" } | ||
| borsh09 = { package = "borsh", version = "=0.9.3" } | ||
| solana-security-txt = { version = "=1.1.1" } | ||
| bytemuck = { version = "=1.16.3", features = ["derive", "min_const_generics"] } | ||
|
|
||
| [dev-dependencies] | ||
| proptest = "1.0" | ||
| serde = "1.0.117" | ||
| serde_json = "1.0.59" | ||
| proptest = "=1.5.0" | ||
| serde = "=1.0.205" | ||
| serde_json = "=1.0.122" |
There was a problem hiding this comment.
Do we need to =x.x.x other than anchor library ?
There was a concern that even fixed versions of Solana would suffer from type mismatches, as different versions of the library would be treated as different libraries.
There was a problem hiding this comment.
This was done because in #172 I added more rust packages in the workspace. These might have different versions of the same libraries. I need to pin them here to the exact versions otherwise it might take a version that is not compatible (but would still satisfy the version constraint here.
The dev dependencies don't matter that much but I figured might as well pin them too. IMO the more of the 'core' dependencies we can pin the less chance there is that in the future it takes a different version which breaks things. Dependabot should ping us when new versions become available which IMO is more transparent than cargo doing those things under the hood.
There was a problem hiding this comment.
hmm, I think the Rust SDK gets requests from developers who want to use with Anchor 0.29, Solana 1.17 or something more up-to-date.
That's what the crate for CPI was. Once anchor released v0.30.0, some devs started asking to update whirlpools crate.
In light of such requests, I think the contract and SDK versions should be intentionally separated.
I think the workspace that ties those two together can be problematic, what do you think?
There was a problem hiding this comment.
Yep agree. If you look at #172 you can see the versions for the rust sdk are more permissive. This is only to prevent any local version mismatches. Any downstream consumer would be able to specify versions they would like to use as long as it complies with the constraints in the rust-sdk cargo file. At least that is how I intended it to be
| pub rent: Sysvar<'info, Rent>, | ||
| /// CHECK: checked via account constraints | ||
| #[account(address = sysvar::rent::ID)] | ||
| pub rent: UncheckedAccount<'info>, |
There was a problem hiding this comment.
hmm, could we keep Sysvar<'info, Rent> ?
To avoid being given arbitrary addresses.
Not as massively executed as "swap" instruction and does not affect transaction size.
As far as open_position is concerned, it should be excluded from open_position_v2. As you know, I think open_position_v2 is clearly preferred because of its advantages (no non-refundable rent overhead).
There was a problem hiding this comment.
The problem is stack space and not transaction size. See my comment below
This change was required because it ran out of stack space in the |
wjthieme
changed the title
# Conflicts: # package.json # yarn.lock
No description provided.