506 switch to stable rust toolchain channel #513

b-yap · 2024-04-19T06:45:09Z

closes #506

mocktopus is a lib for rust nightly alone.
Best to constraint its import in test only, until we plan to fully refactor the code and remove it entirely.
- added a script ~~build-check~~ cmd-all to build (with stable) the project without the currency's testing-utils.
~~💥 the tests in pallet redeem is mocking functions from pallet currency. I had to put a wrapper to be able to mock it properly.~~
- ~~💀 This is an ugly approach, but it works well.~~

How to begin the review:

scripts/~~build-check~~ cmd-all:
- builds all features of all packages except the testing-utils of currency pallet
rust-toolchain.toml
- instead of nightly, use a not-the-latest stable version: 1.77.0
.github/workflows/ci-dev.yml and .github/workflows/ci-main.yml
- need to install the nightly version, and I chose ~~2023-12-29~~ 2024-02-09
  - ❗ the 2023-12-29 had issues ❗ :
    - error[E0658]: use of unstable library feature 'stdsimd'
  - see compile error: use of unstable library feature 'stdsimd' tkaitchuck/aHash#195
    - suggests to use an updated version, hence 2024-02-09
- as stated above, nightly is required for testing.
- fmt only works for nightly
- for building on stable version, use the cmd-all script instead
Cargo.lock:
- had to updated based on the issue stated on the previous point
all README.md s
- all cargo tests need to override rust version with +nightly or a nightly version of your choice
clients/README.md
- forgot to mention from 431 Add environment variable options to Spacewalk #483 to setup environment variables as another option
clients/runner/src/runner.rs
- cleanup not used warnings. (need your help on this @gianfra-t)
clients/stellar-relay-lib/Cargo.toml:
- to fix sole running of tests in stellar-relay-lib,
- need the testing-utils feature of wallet to access the functions for retrieving the secret keys
clients/wallet/src/lib.rs:
- need access to the keys when running tests only for wallet ( cargo test)
clients/wallet/src/keys.rs
- when running the example in stellar-relay-lib, it failed with file not found.
~~pallets/currency/Cargo.toml:~~
- ~~mocktopus only in cfg test.~~
~~pallets/currency/src/amount.rs:~~
- ~~allow mocktopus only in cfg test~~
~~💥 pallets/redeem/src/ext.rs 💥:~~
- Again this is quite ugly but mockable feature of currency's Amount is inaccessible.
- ~~I wrapped it in a trait 🔻 AmountExt 🔻 where we can mock the ff methods of Amount:~~
  - ~~fn lock_on()~~
  - ~~fn burn_from()~~
  - ~~fn unlock_on()~~
  - ~~fn transfer()~~
- implement the trait ^ by calling the actual methods of Amount
~~💥 pallets/redeem/src/lib.rs 💥 :~~
- ~~replace the call of Amount methods, with methods of trait AmountExt:~~
  - ~~fn _lock_on()~~
  - ~~fn _burn_from()~~
  - ~~fn _unlock_on()~~
  - ~~fn _transfer()~~
~~💥 pallets/redeem/src/tests.rs 💥 :~~
- replace the call of Amount methods, with methods of trait AmountExt
all pallets/*/rpc/Cargo.toml:
- jsonrpsee will need the client feature
files not specifically mentioned, are to resolve fmt and clippy issues.

b-yap · 2024-04-24T09:28:46Z

.github/workflows/ci-dev.yml

        # Call `rustup show` as a hack so that the toolchain defined in rust-toolchain.toml is installed
        run: rustup show

+      - name: Remove rust-toolchain.toml for nightly


Installing the nightly (and even setting it to default) will still be overriden by the rust-toolchain.toml file, hence it needs to be removed.
See https://github.com/pendulum-chain/spacewalk/actions/runs/8751187110/job/24020665297#step:9:33

b-yap · 2024-04-24T09:30:04Z

.github/workflows/ci-main.yml

we cannot use --all-targets here because the test must use the nightly version, while the libs and bins must use the stable version

b-yap · 2024-04-24T09:31:15Z

clients/runner/src/runner.rs

@@ -183,6 +183,7 @@ impl Runner {
 			// The set permissions are: -rwx------
 			.mode(0o700)
 			.create(true)
+			.truncate(true)


a warning popped up on nightly version 2024-02-09. truncate(true) was suggested.

I think this is a good suggestion. I just checked at the docs for this option and it will essentially ensure that the opened file is clean, which is what we want since we will save the new binary in there.

In any case at the end of the operation the checksums will be compared, just in case.

b-yap · 2024-04-24T09:31:54Z

clients/runner/src/runner.rs

@@ -515,10 +513,6 @@ impl RunnerExt for Runner {
 		&self.opts.download_path
 	}

-	fn parachain_url(&self) -> String {


This was found to be unused. I also checked, and found it unused anywhere in the code. Please confirm @gianfra-t

We remove some logic from the original runner code, so it makes sense that some methods are not used. I think it is fine!

b-yap · 2024-04-24T09:32:09Z

clients/runner/src/runner.rs

@@ -568,10 +562,6 @@ impl RunnerExt for Runner {
 		Runner::try_load_downloaded_binary(self, release)
 	}

-	async fn get_request_bytes_wrapper(&self, url: String) -> Result<Bytes, Error> {


same case with https://github.com/pendulum-chain/spacewalk/pull/513/files#r1577590058

Also I think it is fine to remove.

b-yap · 2024-04-24T09:32:34Z

clients/stellar-relay-lib/src/connection/connector/message_reader.rs

@@ -80,7 +80,7 @@ async fn read_message_from_stellar(connector: &mut Connector) -> Result<Xdr, Err
 		// 1. the length of the next stellar message
 		// 2. the remaining bytes of the previous stellar message
 		match connector.tcp_stream.read(&mut buff_for_reading).await {
-			Ok(size) if size == 0 => continue,
+			Ok(0) => continue,


suggestion from version nightly-2024-02-09

b-yap · 2024-04-24T09:33:09Z

clients/wallet/src/horizon/horizon.rs

@@ -35,8 +35,7 @@ const BASE_BACKOFF_DELAY_IN_SECS: u64 = 10;
 pub fn horizon_url(is_public_network: bool, is_need_fallback: bool) -> &'static str {
 	if is_public_network {
 		if is_need_fallback {
-			let other_urls =
-				vec!["https://horizon.stellarx.com", "https://horizon.stellar.lobstr.co"];
+			let other_urls = ["https://horizon.stellarx.com", "https://horizon.stellar.lobstr.co"];


suggestion from version nightly-2024-02-09 to use the array directly

b-yap · 2024-04-24T09:34:03Z

clients/wallet/src/keys.rs

@@ -7,7 +7,11 @@ use std::env;
 // previously passed to the environment.
 // A variable not .env but passed to the environment will not be overridden.
 fn get_env_variables(key: &str) -> Option<String> {
-	dotenv::from_filename("../.env").ok();
+	if let None = dotenv::from_filename("../.env").ok() {


had a problem with running the example in stellar-relay-lib.
I moved the env file to the root directory; that's why I added a condition to try looking at the current directory.

b-yap · 2024-04-24T09:34:40Z

clients/wallet/src/lib.rs

@@ -8,7 +8,7 @@ pub use task::*;
 mod cache;
 pub mod error;
 mod horizon;
-#[cfg(feature = "testing-utils")]
+#[cfg(any(test, feature = "testing-utils"))]


keys are also used in the unit test of this lib, so I added test.

pallets/currency/src/amount.rs

b-yap · 2024-04-24T09:39:07Z

pallets/currency/src/mock.rs

@@ -104,11 +102,14 @@ impl pallet_balances::Config for Test {
 parameter_types! {
 	pub const GetNativeCurrencyId: CurrencyId = DEFAULT_NATIVE_CURRENCY;
 	pub const GetRelayChainCurrencyId: CurrencyId = DEFAULT_COLLATERAL_CURRENCY;
-	#[cfg(feature = "runtime-benchmarks")]


this causes error to building/testing other features.
for example:

cargo +nightly test -p vault --features integration-test`

actually fails with a duplicate lang item issue.

cfg macros should be done outside of parameter_types! macros.

b-yap · 2024-04-24T09:39:42Z

pallets/oracle/src/oracle_mock.rs

@@ -15,7 +15,7 @@ pub struct DataKey {

 impl PartialOrd<Self> for DataKey {
 	fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
-		self.blockchain.partial_cmp(&other.blockchain)
+		Some(self.cmp(other))


this is a suggestion in version nightly-2024-02-09

TorstenStueber · 2024-04-24T09:57:12Z

Hey @b-yap, looks like a lot of work went into this. Thanks!

I see that a couple of changes are also pure formatting changes. How is that? Did the formatter settings change or did we not properly use a formatter before?

b-yap · 2024-04-24T12:30:45Z

@TorstenStueber I did not touch the fmt toml file. I think these are all new preferences from the new version?
Like removing ; of a return statement, or removing { } for single statements.

Because I ran cargo +nightly-2024-02-09 fmt --all after the CI failed.

TorstenStueber · 2024-04-24T14:11:29Z

Hmm, is this expected that the default formatting changes between different versions of rust (and therefore rust fmt)? Or did we not properly format before the change?

b-yap · 2024-04-24T15:27:55Z

@TorstenStueber I confirmed it by running
cargo +nightly-2023-04-15 fmt --all on a clean main branch, and there were no changes. We did it properly.

^ In this screenshot, the differences occurred after cargo +nightly-2024-02-09 fmt --all ; the formatting changes are coming from the new version.

TorstenStueber · 2024-04-24T15:41:36Z

Interesting and unexpected. Looking again, there seem to be only a few changes that are only due to formatting. One of them is this and it is clear that this it enacts the trailing_semicolon = false rule. However, trailing_semicolon was already defined to be false before your change (see on main). So it looks like the formatter did not properly format all files before your change. Maybe a bug in that version of rust. But all good now!

gianfra-t · 2024-04-26T14:26:17Z

README.md

@@ -78,7 +78,7 @@ The handling of redeem events is implemented in `clients/vault/src/redeem.rs`.
 ## Run all tests

 ```
-cargo test --lib --features standalone-metadata -- --nocapture
+cargo +nightly --lib --features standalone-metadata -- --nocapture


This should be cargo +nightly test... right? I am not sure about the order

Yes you are right! 🤦🏽‍♀️ My bad.

CI test will be confirmed on #513

ebma

Nice changes 👍

In all the READMEs we now mention +nightly-<some_version>. This is okay and if we want to change it in the future, we can just do a find-and-replace search to change it without much effort. However, I'm still curious if there is a way for us to change it to +nightly but have cargo run it with a specific version. Can we define both stable and nightly in the rust-toolchain.toml or similar so that both versions are fixed?
We also need to make sure that the changes are applied to our other compilation pipeline in the other infrastructure.

.github/workflows/ci-main.yml

clients/README.md

ebma · 2024-04-29T10:48:37Z

clients/README.md

@@ -77,22 +95,24 @@ You can also find an example for setting these variables in the `.env.example` f

 ### Running the tests

+**Note** Tests should run using Rust **_`nightly-2023-12-29`_** version. Make sure to install and add the target `wasm32-unknown-unknown`.


In the top-level README, you put nightly-2024-02-09. Let's try to use the same here as well.

changed to "nightly";

ebma · 2024-04-29T11:55:52Z

pallets/redeem/src/lib.rs

@@ -526,12 +528,12 @@ mod self_redeem {
 	) -> DispatchResult {
 		// burn the tokens that the vault no longer is backing
 		consumed_issued_tokens
-			.lock_on(account_id)
+			._lock_on(account_id)


Can we find a way not to have to call functions with this underscore? I understand you created an extra trait to make this mockable. Can we move that trait next to the actual definition and unify It so that we don't need the underscore?
Otherwise, this creates some confusion and it's not easy to see without context why this is necessary.

I agree; in a few weeks I will have forgotten this.

I have reverted the code to allow mocktopus in pallet currency's feature: testing-utils.
That means cargo b --all-features will not work.

I created a script ~~build-check~~ cmd-all to build all known packages for stable rust version.
testing-utils will only compile on nightly version; or when running cargo test.

pallets/vault-registry/README.md

gianfra-t · 2024-04-29T12:44:04Z

It looks very good 👍!
I am surprised why the mockable macro works different between versions and now it doesn't allow access to the trait anymore. In any case I think the wrapper trait workaround is not that "ugly" and solves the problem.

The only thing remaining to me is to make it work with the --features runtime-benchmarks flag.

…ob/24367603763?pr=513#step:13:15, #513 (comment), #513 (comment),

#513 (review)

b-yap · 2024-04-30T10:28:21Z

@ebma

In all the READMEs we now mention +nightly-<some_version>.

Testing using the latest version of nightly is fine; as long as it's installed. The danger is testing using an older version. I changed to +nightly in the READMEs.

Can we define both stable and nightly in the rust-toolchain.toml or similar so that both versions are fixed?

I checked the profile and there's nothing there.
It's not even possible per package; I doubt it can differentiate between build and test.

I tried cargo aliases but the override is not a "subcommand", so:

     error: no such command: `+nightly`

   Cargo does not handle `+toolchain` directives.
   Did you mean to invoke `cargo` through `rustup` instead?

clients/runner/Cargo.toml

…973897

and use a variable to assign value of the nightly version in CIs

…ob/24367603763?pr=513#step:13:15, #513 (comment), #513 (comment),

#513 (review)

…idually, instead of `--all-features`

…973897

…40/job/24503885678?pr=513#step:11:658

…415117

Co-authored-by: Marcel Ebert <mail@marcel-ebert.de>

…ob/24506180390?pr=513#step:11:3011 cargo build is too heavy; opting with check instead

…ob/24510269896

also added `set -e` to make sure the script exits upon the first error found

…ob/24518930774#step:11:70

…ob/24521111869?pr=513#step:14:20, cleanup the cmd-all command of clippy for `runner`

…w-2038518593

add more comments/explanations

…843698

b-yap · 2024-05-07T06:03:03Z

@ebma @gianfra-t I had to rebase.

ebma

I think we can merge this now once the CI passed.

Great job @b-yap 👌

gianfra-t

Thanks @b-yap looks really good!

ebma · 2024-05-07T14:15:07Z

@zoveress as part of this PR, the CI workflows we do for Spacewalk changed. Do you think you are able to adjust the Gitlab testing pipeline based on the contents of this file? Otherwise please reach out and we are happy to help. The current testing pipeline will not work anymore on Gitlab because we switched to a different Rust toolchain and also had to run the CI a bit differently.

b-yap changed the title ~~switch to stable rust toolchain channel~~ 506 switch to stable rust toolchain channel Apr 19, 2024

b-yap requested a review from a team April 24, 2024 09:24

b-yap commented Apr 24, 2024

View reviewed changes

pallets/currency/src/amount.rs Show resolved Hide resolved

b-yap commented Apr 24, 2024

View reviewed changes

gianfra-t reviewed Apr 26, 2024

View reviewed changes

b-yap added a commit that referenced this pull request Apr 29, 2024

remove file every after use (#515)

9aafee4

CI test will be confirmed on #513

ebma reviewed Apr 29, 2024

View reviewed changes

b-yap added a commit that referenced this pull request Apr 29, 2024

https://github.com/pendulum-chain/spacewalk/actions/runs/8876331480/j…

e3d0644

…ob/24367603763?pr=513#step:13:15, #513 (comment), #513 (comment),

b-yap force-pushed the 506-switch-to-stable-rust-toolchain-channel branch from ebe7d97 to e3d0644 Compare April 29, 2024 13:22

b-yap added a commit that referenced this pull request Apr 30, 2024

no need to define the nightly version

38c1d50

#513 (review)

b-yap commented May 2, 2024

View reviewed changes

clients/runner/Cargo.toml Show resolved Hide resolved

b-yap added a commit that referenced this pull request May 2, 2024

https://github.com/pendulum-chain/spacewalk/pull/513#discussion_r1582…

c95e735

…973897

b-yap requested a review from ebma May 2, 2024 10:19

b-yap and others added 20 commits May 7, 2024 14:01

specify the nightly version in readmes;

f79f777

and use a variable to assign value of the nightly version in CIs

FIX YAML FILE

2eca28d

revert yaml file

7be4923

https://github.com/pendulum-chain/spacewalk/actions/runs/8876331480/j…

ca79253

…ob/24367603763?pr=513#step:13:15, #513 (comment), #513 (comment),

no need to define the nightly version

70edc47

#513 (review)

revert testing-utils of pallet currency, and build packages indiv…

7232fc0

…idually, instead of `--all-features`

https://github.com/pendulum-chain/spacewalk/pull/513#discussion_r1582…

a00739a

…973897

fix https://github.com/pendulum-chain/spacewalk/actions/runs/89221941…

b7e7052

…40/job/24503885678?pr=513#step:11:658

https://github.com/pendulum-chain/spacewalk/pull/513#discussion_r1587…

4948b1e

…415117

Update .github/workflows/ci-dev.yml

f714d73

Co-authored-by: Marcel Ebert <mail@marcel-ebert.de>

https://github.com/pendulum-chain/spacewalk/actions/runs/8922961363/j…

e9c89db

…ob/24506180390?pr=513#step:11:3011 cargo build is too heavy; opting with check instead

https://github.com/pendulum-chain/spacewalk/actions/runs/8924275855/j…

461939e

…ob/24510269896

uncomment all lines

bfec52e

had to remove service as it requires a companion lib: the runtime.

eb2f12d

also added `set -e` to make sure the script exits upon the first error found

https://github.com/pendulum-chain/spacewalk/actions/runs/8926871185/j…

1709860

…ob/24518930774#step:11:70

https://github.com/pendulum-chain/spacewalk/actions/runs/8927530752/j…

fc0729a

…ob/24521111869?pr=513#step:14:20, cleanup the cmd-all command of clippy for `runner`

https://github.com/pendulum-chain/spacewalk/pull/513#pullrequestrevie…

f17cd14

…w-2038518593

fix rewards-distribution test issue

2cd69c2

add more comments/explanations

https://github.com/pendulum-chain/spacewalk/pull/513#discussion_r1590…

20967da

…843698

revise description of cmd-all

1a5c3d6

b-yap force-pushed the 506-switch-to-stable-rust-toolchain-channel branch from 38547a6 to 1a5c3d6 Compare May 7, 2024 06:01

b-yap requested a review from ebma May 7, 2024 06:03

ebma approved these changes May 7, 2024

View reviewed changes

gianfra-t approved these changes May 7, 2024

View reviewed changes

b-yap merged commit 1c3ea1c into main May 7, 2024
2 checks passed

b-yap deleted the 506-switch-to-stable-rust-toolchain-channel branch May 7, 2024 12:40

b-yap mentioned this pull request May 13, 2024

482 add stellar testnet archive urls to vault client config #524

Merged

ebma mentioned this pull request May 13, 2024

Fix pre-commit and pre-push hooks #525

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

506 switch to stable rust toolchain channel #513

506 switch to stable rust toolchain channel #513

b-yap commented Apr 19, 2024 •

edited

Loading

b-yap Apr 24, 2024

b-yap Apr 24, 2024 •

edited

Loading

b-yap Apr 24, 2024 •

edited

Loading

gianfra-t Apr 26, 2024

b-yap Apr 24, 2024

gianfra-t Apr 25, 2024

b-yap Apr 24, 2024

gianfra-t Apr 25, 2024

b-yap Apr 24, 2024

b-yap Apr 24, 2024

b-yap Apr 24, 2024

b-yap Apr 24, 2024

b-yap Apr 24, 2024

b-yap Apr 24, 2024

TorstenStueber commented Apr 24, 2024

b-yap commented Apr 24, 2024 •

edited

Loading

TorstenStueber commented Apr 24, 2024

b-yap commented Apr 24, 2024

TorstenStueber commented Apr 24, 2024

gianfra-t Apr 26, 2024

b-yap Apr 29, 2024

b-yap Apr 29, 2024

ebma left a comment •

edited

Loading

ebma Apr 29, 2024

b-yap May 2, 2024

ebma Apr 29, 2024

b-yap May 2, 2024 •

edited

Loading

gianfra-t commented Apr 29, 2024

b-yap commented Apr 30, 2024 •

edited

Loading

b-yap commented May 7, 2024 •

edited

Loading

ebma left a comment

gianfra-t left a comment

ebma commented May 7, 2024 •

edited

Loading

		@@ -77,22 +95,24 @@ You can also find an example for setting these variables in the `.env.example` f

		### Running the tests

		Note Tests should run using Rust _`nightly-2023-12-29`_ version. Make sure to install and add the target `wasm32-unknown-unknown`.

506 switch to stable rust toolchain channel #513

506 switch to stable rust toolchain channel #513

Conversation

b-yap commented Apr 19, 2024 • edited Loading

How to begin the review:

Choose a reason for hiding this comment

b-yap Apr 24, 2024 • edited Loading

Choose a reason for hiding this comment

b-yap Apr 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TorstenStueber commented Apr 24, 2024

b-yap commented Apr 24, 2024 • edited Loading

TorstenStueber commented Apr 24, 2024

b-yap commented Apr 24, 2024

TorstenStueber commented Apr 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebma left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-yap May 2, 2024 • edited Loading

Choose a reason for hiding this comment

gianfra-t commented Apr 29, 2024

b-yap commented Apr 30, 2024 • edited Loading

b-yap commented May 7, 2024 • edited Loading

ebma left a comment

Choose a reason for hiding this comment

gianfra-t left a comment

Choose a reason for hiding this comment

ebma commented May 7, 2024 • edited Loading

b-yap commented Apr 19, 2024 •

edited

Loading

b-yap Apr 24, 2024 •

edited

Loading

b-yap Apr 24, 2024 •

edited

Loading

b-yap commented Apr 24, 2024 •

edited

Loading

ebma left a comment •

edited

Loading

b-yap May 2, 2024 •

edited

Loading

b-yap commented Apr 30, 2024 •

edited

Loading

b-yap commented May 7, 2024 •

edited

Loading

ebma commented May 7, 2024 •

edited

Loading