Merge pull request #938 from shamil-gadelshin/fix_unstaking_bug

Fix zero unstaking periods in the add_opening() parameters bug.

Cargo.lock

@@ -1569,7 +1569,7 @@ dependencies = [
 
 [[package]]
 name = "joystream-node"
-version = "2.5.0"
+version = "2.6.0"
 dependencies = [
  "ctrlc",
  "derive_more 0.14.1",
@@ -1614,7 +1614,7 @@ dependencies = [
 
 [[package]]
 name = "joystream-node-runtime"
-version = "6.19.0"
+version = "6.20.0"
 dependencies = [
  "parity-scale-codec",
  "safe-mix",
@@ -5568,7 +5568,7 @@ dependencies = [
 
 [[package]]
 name = "substrate-working-group-module"
-version = "1.0.1"
+version = "1.1.0"
 dependencies = [
  "parity-scale-codec",
  "serde",

node/Cargo.toml

@@ -3,7 +3,7 @@ authors = ['Joystream']
 build = 'build.rs'
 edition = '2018'
 name = 'joystream-node'
-version = '2.5.0'
+version = '2.6.0'
 default-run = "joystream-node"
 
 [[bin]]

runtime-modules/working-group/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = 'substrate-working-group-module'
-version = '1.0.1'
+version = '1.1.0'
 authors = ['Joystream contributors']
 edition = '2018'
 

runtime-modules/working-group/src/errors.rs

@@ -265,6 +265,42 @@ decl_error! {
         /// Invalid OpeningPolicyCommitment parameter:
         /// fill_opening_successful_applicant_application_stake_unstaking_period should be non-zero.
         FillOpeningSuccessfulApplicantApplicationStakeUnstakingPeriodIsZero,
+
+        /// Invalid OpeningPolicyCommitment parameter:
+        /// exit_role_stake_unstaking_period should be non-zero.
+        ExitRoleStakeUnstakingPeriodIsZero,
+
+        /// Invalid OpeningPolicyCommitment parameter:
+        /// exit_role_application_stake_unstaking_period should be non-zero.
+        ExitRoleApplicationStakeUnstakingPeriodIsZero,
+
+        /// Invalid OpeningPolicyCommitment parameter:
+        /// terminate_role_stake_unstaking_period should be non-zero.
+        TerminateRoleStakeUnstakingPeriodIsZero,
+
+        /// Invalid OpeningPolicyCommitment parameter:
+        /// terminate_application_stake_unstaking_period should be non-zero.
+        TerminateApplicationStakeUnstakingPeriodIsZero,
+
+        /// Invalid OpeningPolicyCommitment parameter (role_staking_policy):
+        /// crowded_out_unstaking_period_length should be non-zero.
+        RoleStakingPolicyCrowdedOutUnstakingPeriodIsZero,
+
+        /// Invalid OpeningPolicyCommitment parameter (role_staking_policy):
+        /// review_period_expired_unstaking_period_length should be non-zero.
+        RoleStakingPolicyReviewPeriodUnstakingPeriodIsZero,
+
+        /// Invalid OpeningPolicyCommitment parameter (application_staking_policy):
+        /// crowded_out_unstaking_period_length should be non-zero.
+        ApplicationStakingPolicyCrowdedOutUnstakingPeriodIsZero,
+
+        /// Invalid OpeningPolicyCommitment parameter (application_staking_policy):
+        /// review_period_expired_unstaking_period_length should be non-zero.
+        ApplicationStakingPolicyReviewPeriodUnstakingPeriodIsZero,
+
+        /// Invalid OpeningPolicyCommitment parameter (application_rationing_policy):
+        /// max_active_applicants should be non-zero.
+        ApplicationRationingPolicyMaxActiveApplicantsIsZero,
     }
 }
 

runtime-modules/working-group/src/lib.rs

@@ -993,32 +993,92 @@ impl<T: Trait<I>, I: Instance> Module<T, I> {
     fn ensure_opening_policy_commitment_is_valid(
         policy_commitment: &OpeningPolicyCommitment<T::BlockNumber, BalanceOf<T>>,
     ) -> Result<(), Error> {
-        // check fill_opening unstaking periods
-
-        if let Some(unstaking_period) =
-            policy_commitment.fill_opening_failed_applicant_application_stake_unstaking_period
-        {
-            ensure!(
-                unstaking_period != Zero::zero(),
-                Error::FillOpeningFailedApplicantApplicationStakeUnstakingPeriodIsZero
-            );
+        // Helper function. Ensures that unstaking period is None or non-zero.
+        fn check_unstaking_period<BlockNumber: PartialEq + Zero>(
+            unstaking_period: Option<BlockNumber>,
+            error: Error,
+        ) -> Result<(), Error> {
+            if let Some(unstaking_period) = unstaking_period {
+                ensure!(unstaking_period != Zero::zero(), error);
+            }
+            Ok(())
         }
 
-        if let Some(unstaking_period) =
-            policy_commitment.fill_opening_failed_applicant_role_stake_unstaking_period
-        {
-            ensure!(
-                unstaking_period != Zero::zero(),
-                Error::FillOpeningFailedApplicantRoleStakeUnstakingPeriodIsZero
-            );
+        // Helper function. Ensures that unstaking period is None or non-zero in the staking_policy.
+        fn check_staking_policy<Balance, BlockNumber: PartialEq + Zero>(
+            staking_policy: Option<hiring::StakingPolicy<Balance, BlockNumber>>,
+            crowded_out_unstaking_period_error: Error,
+            review_period_unstaking_period_error: Error,
+        ) -> Result<(), Error> {
+            if let Some(staking_policy) = staking_policy {
+                check_unstaking_period(
+                    staking_policy.crowded_out_unstaking_period_length,
+                    crowded_out_unstaking_period_error,
+                )?;
+
+                check_unstaking_period(
+                    staking_policy.review_period_expired_unstaking_period_length,
+                    review_period_unstaking_period_error,
+                )?;
+            }
+
+            Ok(())
         }
 
-        if let Some(unstaking_period) =
-            policy_commitment.fill_opening_successful_applicant_application_stake_unstaking_period
+        // Check all fill_opening unstaking periods.
+        check_unstaking_period(
+            policy_commitment.fill_opening_failed_applicant_role_stake_unstaking_period,
+            Error::FillOpeningFailedApplicantRoleStakeUnstakingPeriodIsZero,
+        )?;
+
+        check_unstaking_period(
+            policy_commitment.fill_opening_failed_applicant_application_stake_unstaking_period,
+            Error::FillOpeningFailedApplicantApplicationStakeUnstakingPeriodIsZero,
+        )?;
+
+        check_unstaking_period(
+            policy_commitment.fill_opening_successful_applicant_application_stake_unstaking_period,
+            Error::FillOpeningSuccessfulApplicantApplicationStakeUnstakingPeriodIsZero,
+        )?;
+
+        check_unstaking_period(
+            policy_commitment.exit_role_stake_unstaking_period,
+            Error::ExitRoleStakeUnstakingPeriodIsZero,
+        )?;
+
+        check_unstaking_period(
+            policy_commitment.exit_role_application_stake_unstaking_period,
+            Error::ExitRoleApplicationStakeUnstakingPeriodIsZero,
+        )?;
+
+        check_unstaking_period(
+            policy_commitment.terminate_role_stake_unstaking_period,
+            Error::TerminateRoleStakeUnstakingPeriodIsZero,
+        )?;
+
+        check_unstaking_period(
+            policy_commitment.terminate_application_stake_unstaking_period,
+            Error::TerminateApplicationStakeUnstakingPeriodIsZero,
+        )?;
+
+        check_staking_policy(
+            policy_commitment.role_staking_policy.clone(),
+            Error::RoleStakingPolicyCrowdedOutUnstakingPeriodIsZero,
+            Error::RoleStakingPolicyReviewPeriodUnstakingPeriodIsZero,
+        )?;
+
+        check_staking_policy(
+            policy_commitment.application_staking_policy.clone(),
+            Error::ApplicationStakingPolicyCrowdedOutUnstakingPeriodIsZero,
+            Error::ApplicationStakingPolicyReviewPeriodUnstakingPeriodIsZero,
+        )?;
+
+        if let Some(application_rationing_policy) =
+            policy_commitment.application_rationing_policy.clone()
         {
             ensure!(
-                unstaking_period != Zero::zero(),
-                Error::FillOpeningSuccessfulApplicantApplicationStakeUnstakingPeriodIsZero
+                application_rationing_policy.max_active_applicants > 0,
+                Error::ApplicationRationingPolicyMaxActiveApplicantsIsZero
             );
         }
 

runtime-modules/working-group/src/tests/mod.rs

@@ -87,6 +87,94 @@ fn add_opening_fails_with_incorrect_unstaking_periods() {
         add_opening_fixture.call_and_assert(Err(
             Error::FillOpeningSuccessfulApplicantApplicationStakeUnstakingPeriodIsZero,
         ));
+
+        let add_opening_fixture =
+            AddWorkerOpeningFixture::default().with_policy_commitment(OpeningPolicyCommitment {
+                exit_role_stake_unstaking_period: Some(0),
+                ..OpeningPolicyCommitment::default()
+            });
+        add_opening_fixture.call_and_assert(Err(Error::ExitRoleStakeUnstakingPeriodIsZero));
+
+        let add_opening_fixture =
+            AddWorkerOpeningFixture::default().with_policy_commitment(OpeningPolicyCommitment {
+                exit_role_application_stake_unstaking_period: Some(0),
+                ..OpeningPolicyCommitment::default()
+            });
+        add_opening_fixture
+            .call_and_assert(Err(Error::ExitRoleApplicationStakeUnstakingPeriodIsZero));
+
+        let add_opening_fixture =
+            AddWorkerOpeningFixture::default().with_policy_commitment(OpeningPolicyCommitment {
+                terminate_role_stake_unstaking_period: Some(0),
+                ..OpeningPolicyCommitment::default()
+            });
+        add_opening_fixture.call_and_assert(Err(Error::TerminateRoleStakeUnstakingPeriodIsZero));
+
+        let add_opening_fixture =
+            AddWorkerOpeningFixture::default().with_policy_commitment(OpeningPolicyCommitment {
+                terminate_application_stake_unstaking_period: Some(0),
+                ..OpeningPolicyCommitment::default()
+            });
+        add_opening_fixture
+            .call_and_assert(Err(Error::TerminateApplicationStakeUnstakingPeriodIsZero));
+
+        let add_opening_fixture =
+            AddWorkerOpeningFixture::default().with_policy_commitment(OpeningPolicyCommitment {
+                role_staking_policy: Some(hiring::StakingPolicy {
+                    crowded_out_unstaking_period_length: Some(0),
+                    ..Default::default()
+                }),
+                ..OpeningPolicyCommitment::default()
+            });
+        add_opening_fixture
+            .call_and_assert(Err(Error::RoleStakingPolicyCrowdedOutUnstakingPeriodIsZero));
+
+        let add_opening_fixture =
+            AddWorkerOpeningFixture::default().with_policy_commitment(OpeningPolicyCommitment {
+                role_staking_policy: Some(hiring::StakingPolicy {
+                    review_period_expired_unstaking_period_length: Some(0),
+                    ..Default::default()
+                }),
+                ..OpeningPolicyCommitment::default()
+            });
+        add_opening_fixture.call_and_assert(Err(
+            Error::RoleStakingPolicyReviewPeriodUnstakingPeriodIsZero,
+        ));
+
+        let add_opening_fixture =
+            AddWorkerOpeningFixture::default().with_policy_commitment(OpeningPolicyCommitment {
+                application_staking_policy: Some(hiring::StakingPolicy {
+                    crowded_out_unstaking_period_length: Some(0),
+                    ..Default::default()
+                }),
+                ..OpeningPolicyCommitment::default()
+            });
+        add_opening_fixture.call_and_assert(Err(
+            Error::ApplicationStakingPolicyCrowdedOutUnstakingPeriodIsZero,
+        ));
+
+        let add_opening_fixture =
+            AddWorkerOpeningFixture::default().with_policy_commitment(OpeningPolicyCommitment {
+                application_staking_policy: Some(hiring::StakingPolicy {
+                    review_period_expired_unstaking_period_length: Some(0),
+                    ..Default::default()
+                }),
+                ..OpeningPolicyCommitment::default()
+            });
+        add_opening_fixture.call_and_assert(Err(
+            Error::ApplicationStakingPolicyReviewPeriodUnstakingPeriodIsZero,
+        ));
+
+        let add_opening_fixture =
+            AddWorkerOpeningFixture::default().with_policy_commitment(OpeningPolicyCommitment {
+                application_rationing_policy: Some(hiring::ApplicationRationingPolicy {
+                    max_active_applicants: 0,
+                }),
+                ..OpeningPolicyCommitment::default()
+            });
+        add_opening_fixture.call_and_assert(Err(
+            Error::ApplicationRationingPolicyMaxActiveApplicantsIsZero,
+        ));
     });
 }
 

runtime/Cargo.toml

@@ -4,7 +4,7 @@ edition = '2018'
 name = 'joystream-node-runtime'
 # Follow convention: https://github.com/Joystream/substrate-runtime-joystream/issues/1
 # {Authoring}.{Spec}.{Impl} of the RuntimeVersion
-version = '6.19.0'
+version = '6.20.0'
 
 [features]
 default = ['std']

runtime/src/lib.rs

@@ -161,7 +161,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
     spec_name: create_runtime_str!("joystream-node"),
     impl_name: create_runtime_str!("joystream-node"),
     authoring_version: 6,
-    spec_version: 19,
+    spec_version: 20,
     impl_version: 0,
     apis: RUNTIME_API_VERSIONS,
 };