-
Notifications
You must be signed in to change notification settings - Fork 293
upgrade contract anchor version to 0.31.1 #1081
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
| @@ -1,5 +1,5 @@ | |||
| use anchor_lang::prelude::*; | |||
| use anchor_lang::solana_program::pubkey; | |||
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.
It seems like most places we rely directly on our pinned version of solana-program. If this import was intentional, I will revert.
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 really need solana_program as a separate dependency if anchor already exposes solana_program? Can we get rid of the direct solana_program dep if we use anchor_lang::solana_program everywhere?
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.
At first I removed the direct dependency, but then I figured there was a specific reason we had it. In the spirit of making as few changes as possible, I kept the direct dependency. I'm happy to remove if there's no issue.
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.
Don't think it matters that much, deps such as spl-token also still require solana-program and the version needs to be in sync between all the deps. Let's just keep it like it is in this PR and not bother as it won't benefit us much
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.
+1 for removing direct dep on solana_program, but also agree that it should be handled in another PR to reduce diffs.
Could I ask what is "a specific reason we had 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.
Could I ask what is "a specific reason we had it".
I'm not sure, to be honest. But since I haven't worked on Whirlpools before, I didn't want to just remove without first confirming there is no issue with doing so.
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 I removed spl-token and spl-token-metadata-interface, what do you think about just removing solana-program now? I'm fine to leave as-is, but it could be convenient to just yank it now.
| "version": "0.6.0", | ||
| "scripts": { | ||
| "build": "RUSTUP_TOOLCHAIN=1.78.0 anchor build -p whirlpool", | ||
| "build": "anchor build -p whirlpool", |
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 could not get RUSTUP_TOOLCHAIN=1.xx.xx anchor build -p whirlpool to work. I tried many different versions of the rust toolchain and directly compared against my local toolchain. Even when using the same version, I got the error below. I tried troubleshooting for a while but ultimately decided to just open the PR and get feedback on this proposed update.
Error
error[E0277]: the trait bound `PacketFlags: Deserialize<'_>` is not satisfied
--> /Users/jacobshiohira/.cargo/registry/src/index.crates.io-6f17d22bba15001f/solana-packet-2.1.0/src/lib.rs:51:38
|
51 | #[cfg_attr(feature = "serde", derive(Deserialize, Serialize))]
| ^^^^^^^^^^^ the trait `Deserialize<'_>` is not implemented for `PacketFlags`
|
= note: for local types consider adding `#[derive(serde::Deserialize)]` to your `PacketFlags` type
= note: for types from other crates check whether the crate offers a `serde` feature flag
= help: the following other types implement trait `Deserialize<'de>`:
&'a Path
&'a [u8]
&'a str
()
(T,)
(T0, T1)
(T0, T1, T2)
(T0, T1, T2, T3)
and 147 others
note: required by a bound in `next_element`
--> /Users/jacobshiohira/.cargo/registry/src/index.crates.io-6f17d22bba15001f/serde-1.0.213/src/de/mod.rs:1732:12
|
1730 | fn next_element<T>(&mut self) -> Result<Option<T>, Self::Error>
| ------------ required by a bound in this associated function
1731 | where
1732 | T: Deserialize<'de>,
| ^^^^^^^^^^^^^^^^ required by this bound in `SeqAccess::next_element`
= note: this error originates in the derive macro `Deserialize` (in Nightly builds, run with -Z macro-backtrace for more info)
error[E0277]: the trait bound `PacketFlags: Deserialize<'_>` is not satisfied
--> /Users/jacobshiohira/.cargo/registry/src/index.crates.io-6f17d22bba15001f/solana-packet-2.1.0/src/lib.rs:51:38
|
51 | #[cfg_attr(feature = "serde", derive(Deserialize, Serialize))]
| ^^^^^^^^^^^ the trait `Deserialize<'_>` is not implemented for `PacketFlags`
|
= note: for local types consider adding `#[derive(serde::Deserialize)]` to your `PacketFlags` type
= note: for types from other crates check whether the crate offers a `serde` feature flag
= help: the following other types implement trait `Deserialize<'de>`:
&'a Path
&'a [u8]
&'a str
()
(T,)
(T0, T1)
(T0, T1, T2)
(T0, T1, T2, T3)
and 147 others
note: required by a bound in `next_value`
--> /Users/jacobshiohira/.cargo/registry/src/index.crates.io-6f17d22bba15001f/serde-1.0.213/src/de/mod.rs:1871:12
|
1869 | fn next_value<V>(&mut self) -> Result<V, Self::Error>
| ---------- required by a bound in this associated function
1870 | where
1871 | V: Deserialize<'de>,
| ^^^^^^^^^^^^^^^^ required by this bound in `MapAccess::next_value`
= note: this error originates in the derive macro `Deserialize` (in Nightly builds, run with -Z macro-backtrace for more info)
error[E0277]: the trait bound `PacketFlags: Serialize` is not satisfied
--> /Users/jacobshiohira/.cargo/registry/src/index.crates.io-6f17d22bba15001f/solana-packet-2.1.0/src/lib.rs:51:51
|
51 | #[cfg_attr(feature = "serde", derive(Deserialize, Serialize))]
| ^^^^^^^^^ the trait `Serialize` is not implemented for `PacketFlags`
|
= note: for local types consider adding `#[derive(serde::Serialize)]` to your `PacketFlags` type
= note: for types from other crates check whether the crate offers a `serde` feature flag
= help: the following other types implement trait `Serialize`:
&'a T
&'a mut T
()
(T,)
(T0, T1)
(T0, T1, T2)
(T0, T1, T2, T3)
(T0, T1, T2, T3, T4)
and 133 others
note: required by a bound in `_::_serde::ser::SerializeStruct::serialize_field`
--> /Users/jacobshiohira/.cargo/registry/src/index.crates.io-6f17d22bba15001f/serde-1.0.213/src/ser/mod.rs:1867:21
|
1865 | fn serialize_field<T>(&mut self, key: &'static str, value: &T) -> Result<(), Self::Error>
| --------------- required by a bound in this associated function
1866 | where
1867 | T: ?Sized + Serialize;
| ^^^^^^^^^ required by this bound in `SerializeStruct::serialize_field`
= note: this error originates in the derive macro `Serialize` (in Nightly builds, run with -Z macro-backtrace for more info)
For more information about this error, try `rustc --explain E0277`.
error: could not compile `solana-packet` (lib) due to 5 previous errors| pub use tick_array::*; | ||
| pub use token_badge::*; | ||
| pub use zeroed_tick_array::*; | ||
| pub(crate) use zeroed_tick_array::*; |
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.
from compiler warning
Compiling whirlpool v0.6.0 (/Users/jacobshiohira/projects/solana/orca/whirlpools/programs/whirlpool)
warning: glob import doesn't reexport anything with visibility `pub` because no imported item is public enough
--> programs/whirlpool/src/state/mod.rs:31:9
|
31 | pub use zeroed*tick_array::*;
| ^^^^^^^^^^^^^^^^^^^^
|
note: the most public imported item is `pub(crate)`
--> programs/whirlpool/src/state/mod.rs:31:9
|
31 | pub use zeroed*tick_array::*;
| ^^^^^^^^^^^^^^^^^^^^
= help: reduce the glob import's visibility or increase visibility of imported items
= note: `#[warn(unused_imports)]` on by default
Would be great to add this as a regression test. Can you do in a separate PR (we can merge that directly into main before merging this) |
wjthieme
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.
Awesome! Couple small remarks but the gist LGTM
| @@ -1,5 +1,5 @@ | |||
| use anchor_lang::prelude::*; | |||
| use anchor_lang::solana_program::pubkey; | |||
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 really need solana_program as a separate dependency if anchor already exposes solana_program? Can we get rid of the direct solana_program dep if we use anchor_lang::solana_program everywhere?
| StateWithExtensions::<spl_token_2022::state::Mint>::unpack(&token_mint_data)?; | ||
| let new_account_len = token_mint_unpacked | ||
| .try_get_new_account_len::<spl_token_metadata_interface::state::TokenMetadata>(&metadata)?; | ||
| .try_get_new_account_len_for_variable_len_extension::<spl_token_metadata_interface::state::TokenMetadata>(&metadata)?; |
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.
memo: breaking change on spl-token-2022-1.0.0 (0.9.0 -> 1.0.0)
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.
Noted, thank you! Is there a change you are requesting as a result of this?
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.
ah sorry, this is not change request. I believe your update is correct of course.
Just a memo for reviewers. 😅
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.
Haha ok, no worries! I just wanted to make sure that I didn't miss any change requests. I really appreciate the comment - I think more context is always helpful 🙌
|
Based on slack logs, I believe you've already done |
yugure-orca
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 think removing direct deps on spl-token and spl-token-metadata-interface is possible in thie PR because the changes are very small (~10 lines only)
| @@ -1,5 +1,5 @@ | |||
| use anchor_lang::prelude::*; | |||
| use anchor_lang::solana_program::pubkey; | |||
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.
+1 for removing direct dep on solana_program, but also agree that it should be handled in another PR to reduce diffs.
Could I ask what is "a specific reason we had it".
|
Integration tests (anchor tests) check all instructions, and since the changes aren’t that many, I think it should be fine. But in the end, I’d like to build more confidence by running a regression test that executes a few days’ worth of transactions with both the current binary (v0.29.0) and the new binary (v0.31.1). https://github.com/orca-so/whirlpool-tx-replayer/tree/main/whirlpool-regression-test-command |
Yes, I think that makes sense. I didn't originally add it since these tests depend on some changes in |
Ooh, I didn't realize this testing tool existed - thank you for linking! I will look into this and follow up with results 🗒️ Update: what a cool tool! I got this working, currently running the command against 10/01 transactions and will go from there. |
programs/whirlpool/Cargo.toml
Outdated
| arrayref = { version = "=0.3.8" } | ||
| borsh09 = { package = "borsh", version = "=0.9.3" } | ||
| arrayref = { version = "=0.3.9" } | ||
| borsh = { version = "=0.10.3" } |
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.
It seems like we no longer need a separate borsh import after removing impl_borsh_serialize_for_bn and impl_borsh_deserialize_for_bn. If there are no concerns, I can remove this dependency as well.
hey @yugure-orca, i wanted to let you know i ran your tool over data from 10/01 -> 10/06 without regression. would you like me to further run the regression test tool - include more days, test a specific day in particular, etc? |
896a2e9 to
265101b
Compare
265101b to
4ea3c7f
Compare
Description
discriminator()was removed in favor of::DISCRIMINATORwhich is a variable slice because discriminators can of length != 8 now. Most changes are related to this.Proposed PR process
There are a lot of changes related to this upgrade (>100 files). My plan is to split up the changes into smaller PRs, and merge everything into
TKN-700/base. After each PR has been reviewed, approved, and merged into the base branch, we can easily merge into main.Testing
anchor build -p whirlpoolcargo test -p whirlpool --libanchor test --run anchor_tests/int/*andanchor test --run anchor_tests/sdk/*legacy-sdk: update legacy-sdk #1088