pendulum-chain / spacewalk

Apache License 2.0
34 stars 7 forks source link

506 switch to stable rust toolchain channel #513

Closed b-yap closed 4 months ago

b-yap commented 4 months ago

closes #506

  1. scripts/~build-check~ cmd-all:
    • builds all features of all packages except the testing-utils of currency pallet
  2. rust-toolchain.toml
    • instead of nightly, use a not-the-latest stable version: 1.77.0
  3. .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
    • as stated above, nightly is required for testing.
    • fmt only works for nightly
    • for building on stable version, use the cmd-all script instead
  4. Cargo.lock:
    • had to updated based on the issue stated on the previous point
  5. all README.md s
    • all cargo tests need to override rust version with +nightly or a nightly version of your choice
  6. clients/README.md
  7. clients/runner/src/runner.rs
    • cleanup not used warnings. (need your help on this @gianfra-t)
      1. 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
  8. clients/wallet/src/lib.rs:
    • need access to the keys when running tests only for wallet ( cargo test)
  9. clients/wallet/src/keys.rs
    • when running the example in stellar-relay-lib, it failed with file not found.
  10. ~pallets/currency/Cargo.toml:~
    • ~mocktopus only in cfg test.~
  11. ~pallets/currency/src/amount.rs:~
    • ~allow mocktopus only in cfg test~
  12. ~:boom: pallets/redeem/src/ext.rs :boom::~
    • ~Again this is quite ugly but mockable feature of currency's Amount is inaccessible.~
    • ~I wrapped it in a trait :small_red_triangle_down: AmountExt :small_red_triangledown: 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~
  13. ~:boom: pallets/redeem/src/lib.rs :boom: :~
    • ~replace the call of Amount methods, with methods of trait AmountExt:~
      • ~fn _lock_on()~
      • ~fn _burn_from()~
      • ~fn _unlock_on()~
      • ~fn _transfer()~
  14. ~:boom: pallets/redeem/src/tests.rs :boom: :~
    • ~replace the call of Amount methods, with methods of trait AmountExt~
  15. all pallets/*/rpc/Cargo.toml:
    • jsonrpsee will need the client feature
  16. files not specifically mentioned, are to resolve fmt and clippy issues.
TorstenStueber commented 4 months ago

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 commented 4 months ago

@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 commented 4 months ago

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 commented 4 months ago

@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.

Screenshot 2024-04-24 at 11 25 08 PM

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

TorstenStueber commented 4 months ago

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 commented 4 months ago

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.

b-yap commented 4 months ago

@ebma

In all the READMEs we now mention +nightly-.

b-yap commented 4 months ago

@ebma @gianfra-t I had to rebase.

ebma commented 4 months ago

@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.