ci improvements (#207)

* ci improvements * indent * indent * set executable * install protobuf * fmt * clippy 1 * fix * fmt * install wasm toolchain * run clippy on runtime-benchmarks * clippy 2 * fix obscure error * fmt * fix license * fmt, clippy * fmt * grant benchmarks error * fmt * same thing
ImbueNetwork · Sep 11, 2023 · 7e4e9ea · 7e4e9ea
1 parent 6faf3a3
commit 7e4e9ea
Show file tree

Hide file tree

Showing 35 changed files with 303 additions and 262 deletions.
diff --git a/.github/workflows/collator_actions.yml b/.github/workflows/collator_actions.yml
@@ -1,4 +1,4 @@
-name: build-imbue-collator
+name: rust
 on:
   workflow_dispatch:
   push:
@@ -12,12 +12,15 @@ on:
     paths-ignore:
       - "**.md"
 
+    env: 
+      CARGO_TERM_COLOR: always
+
 jobs:
   build-imbue-collator:
     runs-on: ubuntu-latest
     steps:
       - name: Checkout sources
-        uses: actions/checkout@v2
+        uses: actions/checkout@v4
         with:
           submodules: recursive
 
@@ -40,3 +43,32 @@ jobs:
 
       - name: Run tests with benchmarks
         run: cargo test --features runtime-benchmarks
+  clippy:
+      runs-on: ubuntu-latest
+      steps:
+      - uses: actions/checkout@v4
+      - name: Install Protobuf
+        run: sudo apt install protobuf-compiler
+      - name: Install minimal nightly Rust
+        uses: actions-rs/toolchain@v1
+        with:
+          profile: minimal
+          toolchain: nightly-2023-05-22
+          target: wasm32-unknown-unknown
+          override: true
+      - name: Install clippy
+        run: rustup component add clippy
+      - name: Ensure executable
+        run: chmod +x ./ci/jobs/clippy.sh
+      - name: Run clippy
+        run: ./ci/jobs/clippy.sh
+  rustfmt:
+    runs-on: ubuntu-latest
+    steps:
+    - uses: actions/checkout@v4
+    - name: Install rustfmt
+      run: rustup component add rustfmt
+    - name: Ensure executable
+      run: chmod +x ./ci/jobs/rustfmt.sh
+    - name: Run rustfmt
+      run: ./ci/jobs/rustfmt.sh
diff --git a/.github/workflows/pull_request_template.md b/.github/workflows/pull_request_template.md
@@ -0,0 +1,14 @@
+# Goal
+The goal of this PR is <!-- insert goal here -->
+
+Closes <!-- issue # -->
+
+# Discussion
+<!-- List discussion items -->
+
+# Checklist
+- [ ] Chain spec updated
+- [ ] Custom RPC OR Runtime API added/changed? Updated js/api-augment.
+- [ ] Tests added
+- [ ] Benchmarks added
+- [ ] Weights updated
diff --git a/ci/jobs/clippy.sh b/ci/jobs/clippy.sh
@@ -0,0 +1,7 @@
+#!/bin/bash
+set -euo pipefail
+cd -- "$(dirname -- "${BASH_SOURCE[0]}")"
+cd ../..
+
+echo "Starting clippy!"
+RUSTFLAGS="-D warnings" cargo clippy --all --features runtime-benchmarks
diff --git a/ci/jobs/rustfmt.sh b/ci/jobs/rustfmt.sh
@@ -0,0 +1,7 @@
+#!/bin/bash
+set -euo pipefail
+cd -- "$(dirname -- "${BASH_SOURCE[0]}")"
+cd ../..
+
+echo "Starting cargo fmt!"
+cargo fmt --check --all
diff --git a/node/src/chain_spec.rs b/node/src/chain_spec.rs
@@ -247,7 +247,7 @@ fn development_genesis(
                 .cloned()
                 .map(|(acc, _)| acc)
                 .collect(),
-            candidacy_bond: 1 * IMBU,
+            candidacy_bond: IMBU,
             ..Default::default()
         },
         council_membership: CouncilMembershipConfig {

diff --git a/node/src/cli.rs b/node/src/cli.rs
@@ -2,6 +2,7 @@ use clap::Parser;
 use std::path::PathBuf;
 
 /// Sub-commands supported by the collator.
+#[allow(clippy::large_enum_variant)]
 #[derive(Debug, Parser)]
 pub enum Subcommand {
     /// Export the genesis state of the parachain.

diff --git a/node/src/rpc.rs b/node/src/rpc.rs
@@ -58,6 +58,6 @@ where
 
     module.merge(System::new(client.clone(), pool, deny_unsafe).into_rpc())?;
     module.merge(TransactionPayment::new(client.clone()).into_rpc())?;
-    module.merge(Proposals::new(client.clone()).into_rpc())?;
+    module.merge(Proposals::new(client).into_rpc())?;
     Ok(module)
 }
diff --git a/node/src/service.rs b/node/src/service.rs
@@ -58,6 +58,7 @@ type ParachainBlockImport = TParachainBlockImport<Block, Arc<ParachainClient>, P
 ///
 /// Use this macro if you don't actually need the full service, but just the builder in order to
 /// be able to perform chain operations.
+#[allow(clippy::type_complexity)]
 pub fn new_partial(
     config: &Configuration,
 ) -> Result<
@@ -86,12 +87,7 @@ pub fn new_partial(
         })
         .transpose()?;
 
-    let executor = ParachainExecutor::new(
-        config.wasm_method,
-        config.default_heap_pages,
-        config.max_runtime_instances,
-        config.runtime_cache_size,
-    );
+    let executor = sc_service::new_native_or_wasm_executor(config);
 
     let (client, backend, keystore_container, task_manager) =
         sc_service::new_full_parts::<Block, RuntimeApi, _>(
@@ -354,6 +350,7 @@ fn build_import_queue(
         .map_err(Into::into)
 }
 
+#[allow(clippy::too_many_arguments)]
 fn build_consensus(
     client: Arc<ParachainClient>,
     block_import: ParachainBlockImport,

diff --git a/pallets/briefs/src/benchmarking.rs b/pallets/briefs/src/benchmarking.rs
@@ -38,7 +38,7 @@ mod benchmarks {
             applicant,
             budget,
             initial_contribution,
-            brief_id.clone(),
+            brief_id,
             CurrencyId::Native,
             milestones,
         );
@@ -56,12 +56,12 @@ mod benchmarks {
         let brief_id = gen_hash(1);
         let milestones = get_max_milestones::<T>();
         assert_ok!(Briefs::<T>::create_brief(
-            RawOrigin::Signed(caller.clone()).into(),
+            RawOrigin::Signed(caller).into(),
             brief_owners.clone(),
             applicant,
             budget,
             initial_contribution,
-            brief_id.clone(),
+            brief_id,
             CurrencyId::Native,
             milestones
         ));
@@ -70,7 +70,7 @@ mod benchmarks {
         #[extrinsic_call]
         contribute_to_brief(
             RawOrigin::Signed(brief_owner.clone()),
-            brief_id.clone(),
+            brief_id,
             contribution,
         );
         assert_last_event::<T>(Event::<T>::BriefContribution(brief_owner, brief_id).into());
@@ -86,18 +86,18 @@ mod benchmarks {
         let brief_id = gen_hash(1);
         let milestones = get_max_milestones::<T>();
         assert_ok!(Briefs::<T>::create_brief(
-            RawOrigin::Signed(caller.clone()).into(),
+            RawOrigin::Signed(caller).into(),
             brief_owners,
             applicant.clone(),
             budget,
             initial_contribution,
-            brief_id.clone(),
+            brief_id,
             CurrencyId::Native,
             milestones
         ));
         // (origin, brief_id)
         #[extrinsic_call]
-        commence_work(RawOrigin::Signed(applicant), brief_id.clone());
+        commence_work(RawOrigin::Signed(applicant), brief_id);
         assert_last_event::<T>(Event::<T>::BriefEvolution(brief_id).into());
     }
 
@@ -113,16 +113,16 @@ mod benchmarks {
         assert_ok!(Briefs::<T>::create_brief(
             RawOrigin::Signed(caller.clone()).into(),
             brief_owners,
-            applicant.clone(),
+            applicant,
             budget,
             initial_contribution,
-            brief_id.clone(),
+            brief_id,
             CurrencyId::Native,
             milestones
         ));
         // (origin, brief_id)
         #[extrinsic_call]
-        cancel_brief(RawOrigin::Signed(caller), brief_id.clone());
+        cancel_brief(RawOrigin::Signed(caller), brief_id);
         assert_last_event::<T>(Event::<T>::BriefCanceled(brief_id).into());
     }
 
@@ -135,7 +135,7 @@ mod benchmarks {
 
 fn create_account_id<T: Config>(suri: &'static str, n: u32) -> T::AccountId {
     let user = account(suri, n, SEED);
-    let initial_balance: _ = 1_000_000_000_000_000u128;
+    let initial_balance = 1_000_000_000_000_000u128;
     assert_ok!(T::RMultiCurrency::deposit(
         CurrencyId::Native,
         &user,
@@ -177,15 +177,14 @@ fn get_milestones<T: Config>(mut n: u32) -> BoundedProposedMilestones<T> {
     if n > max {
         n = max;
     }
-    let milestones = (0..n)
+
+    (0..n)
         .map(|_| ProposedMilestone {
             percentage_to_unlock: Percent::from_percent((100 / n) as u8),
         })
         .collect::<Vec<ProposedMilestone>>()
         .try_into()
-        .expect("qed");
-
-    milestones
+        .expect("qed")
 }
 
 fn get_max_milestones<T: Config>() -> BoundedProposedMilestones<T> {

diff --git a/pallets/briefs/src/integration_tests.rs b/pallets/briefs/src/integration_tests.rs
@@ -19,7 +19,7 @@ fn create_proposal_from_brief() {
             *ALICE,
             contribution_value,
             contribution_value,
-            brief_id.clone(),
+            brief_id,
             CurrencyId::Native,
             get_milestones(10),
         );

diff --git a/pallets/briefs/src/lib.rs b/pallets/briefs/src/lib.rs
@@ -101,19 +101,14 @@ pub mod pallet {
     #[pallet::storage]
     pub type StorageVersion<T: Config> = StorageValue<_, Release, ValueQuery>;
 
-    #[derive(Encode, Decode, TypeInfo, PartialEq, MaxEncodedLen)]
+    #[derive(Encode, Decode, TypeInfo, PartialEq, MaxEncodedLen, Default)]
     #[repr(u32)]
     pub enum Release {
         V0,
+        #[default]
         V1,
     }
 
-    impl Default for Release {
-        fn default() -> Release {
-            Release::V0
-        }
-    }
-
     #[pallet::event]
     #[pallet::generate_deposit(pub(super) fn deposit_event)]
     pub enum Event<T: Config> {
@@ -175,6 +170,7 @@ pub mod pallet {
     impl<T: Config> Pallet<T> {
         /// Create a brief to be funded or amended.
         /// In the current state the applicant must be approved.
+        #[allow(clippy::too_many_arguments)]
         #[pallet::call_index(2)]
         #[pallet::weight(<T as Config>::WeightInfo::create_brief())]
         pub fn create_brief(
@@ -344,7 +340,7 @@ pub mod pallet {
             <T as Config>::DepositHandler::return_deposit(brief.deposit_id)?;
             let contributions = BriefContributions::<T>::get(brief_id);
             for (who, c) in contributions.iter() {
-                <T as Config>::RMultiCurrency::unreserve(brief.currency_id, &who, c.value);
+                <T as Config>::RMultiCurrency::unreserve(brief.currency_id, who, c.value);
             }
 
             BriefContributions::<T>::remove(brief_id);

diff --git a/pallets/briefs/src/migrations.rs b/pallets/briefs/src/migrations.rs
@@ -46,7 +46,7 @@ pub(crate) mod v1 {
                 let maybe_milestones: Result<BoundedProposedMilestones<T>, _> = brief
                     .milestones
                     .iter()
-                    .map(|ms| {
+                    .filter_map(|ms| {
                         let convert: Result<u8, _> = ms.percentage_to_unlock.try_into();
                         if let Ok(n) = convert {
                             Some(ProposedMilestone {
@@ -56,7 +56,6 @@ pub(crate) mod v1 {
                             None
                         }
                     })
-                    .flatten()
                     .collect::<Vec<ProposedMilestone>>()
                     .try_into();
 
@@ -92,6 +91,7 @@ mod test {
     #[test]
     fn migrate_v0_to_v1() {
         build_test_externality().execute_with(|| {
+            crate::StorageVersion::<Test>::put(Release::V0);
             let milestones: BoundedVec<
                 v0::ProposedMilestoneV0,
                 <Test as Config>::MaxMilestonesPerBrief,

diff --git a/pallets/briefs/src/mock.rs b/pallets/briefs/src/mock.rs
@@ -202,10 +202,10 @@ impl DepositHandler<Balance, AccountId> for MockDepositHandler {
         Ok(0u64)
     }
     fn return_deposit(_deposit_id: Self::DepositId) -> DispatchResult {
-        Ok(().into())
+        Ok(())
     }
     fn slash_reserve_deposit(_deposit_id: Self::DepositId) -> DispatchResult {
-        Ok(().into())
+        Ok(())
     }
 }
 

diff --git a/pallets/briefs/src/tests.rs b/pallets/briefs/src/tests.rs
@@ -258,7 +258,7 @@ fn reserved_funds_are_transferred_to_project_kitty() {
             *ALICE,
             contribution_value,
             contribution_value,
-            brief_id.clone(),
+            brief_id,
             CurrencyId::Native,
             get_milestones(10),
         );
@@ -286,11 +286,11 @@ fn cancel_brief_works() {
 
         assert_ok!(BriefsMod::create_brief(
             RuntimeOrigin::signed(*ALICE),
-            owners.clone(),
+            owners,
             *BOB,
             budget,
             initial_contribution,
-            brief_id.clone(),
+            brief_id,
             CurrencyId::Native,
             get_milestones(10),
         ));
@@ -356,7 +356,7 @@ fn cancel_brief_not_brief_owner() {
             *ALICE,
             contribution_value,
             contribution_value,
-            brief_id.clone(),
+            brief_id,
             CurrencyId::Native,
             get_milestones(10),
         ));
@@ -394,13 +394,12 @@ pub(crate) fn get_milestones(mut n: u32) -> BoundedProposedMilestones<Test> {
     if n > max {
         n = max;
     }
-    let milestones = (0..n)
+
+    (0..n)
         .map(|_| ProposedMilestone {
             percentage_to_unlock: Percent::from_percent((100 / n) as u8),
         })
         .collect::<Vec<ProposedMilestone>>()
         .try_into()
-        .expect("qed");
-
-    milestones
+        .expect("qed")
 }
diff --git a/pallets/crowdfunding/README.md b/pallets/crowdfunding/README.md