From e97d36e502d8ded02fb27a0990b59ebebbe8c8dc Mon Sep 17 00:00:00 2001 From: didiermis Date: Mon, 18 Mar 2024 15:14:35 -0600 Subject: [PATCH 1/7] - Introduced a new helper function `validate_fields` in the Gated Marketplace pallet to validate the fields and custodian fields inputs before setting up an application. It throws an `InsufficientCustodianFields` error if the lengths of `fields` and `custodian_fields` do not match, and a `FieldsNotProvided` error if custodian fields are absent but fields are present. - Incorporated the newly-created `validate_fields` helper function into the `create_marketplace` and `enroll_marketplace` functions in the Gated Marketplace pallet's `#[pallet::call]` section. This addition incorporates additional checks and enhances data accuracy. Also introduced a new type of error, `FieldsNotProvided`, in the pallet to handle scenarios where fields are not provided for the application. - Performed a minor cleanup in the RBAC pallet configuration in mock.rs, reordering the fields for better clarity. Swapped the places of `RemoveOrigin` and `WeightInfo` fields to maintain consistent code order. No functionality was changed in this commit. --- pallets/gated-marketplace/src/functions.rs | 20 +++++++++++++++++++- pallets/gated-marketplace/src/lib.rs | 6 ++++++ pallets/gated-marketplace/src/mock.rs | 17 +++++++++-------- 3 files changed, 34 insertions(+), 9 deletions(-) diff --git a/pallets/gated-marketplace/src/functions.rs b/pallets/gated-marketplace/src/functions.rs index f3ad44aa..e3f57a80 100644 --- a/pallets/gated-marketplace/src/functions.rs +++ b/pallets/gated-marketplace/src/functions.rs @@ -135,6 +135,7 @@ impl Pallet { // ensure the origin is owner or admin Self::is_authorized(authority.clone(), &marketplace_id, Permission::Enroll)?; + Self::validate_fields(&fields, &custodian_fields)?; let (custodian, fields) = Self::set_up_application(fields, custodian_fields); let application = Application:: { @@ -794,6 +795,24 @@ impl Pallet { } /* ---- Helper functions ---- */ + pub fn validate_fields( + fields: &Fields, + custodian_fields: &Option>, + ) -> DispatchResult { + match custodian_fields { + Some(c_fields) => { + if fields.len() != c_fields.1.len() { + return Err(Error::::InsufficientCustodianFields.into()) + } + } + None => { + if !fields.is_empty() { + return Err(Error::::FieldsNotProvided.into()) + } + } + } + Ok(()) + } pub fn set_up_application( fields: Fields, @@ -812,7 +831,6 @@ impl Pallet { for (i, field) in f.iter_mut().enumerate() { field.custodian_cid = Some(c_fields.1[i].clone()); } - Some(c_fields.0) }, _ => None, diff --git a/pallets/gated-marketplace/src/lib.rs b/pallets/gated-marketplace/src/lib.rs index edca593b..e6c55d85 100644 --- a/pallets/gated-marketplace/src/lib.rs +++ b/pallets/gated-marketplace/src/lib.rs @@ -338,6 +338,10 @@ pub mod pallet { OwnerNotInMarketplace, /// MappedAssetId not found AssetNotFound, + /// Not enough custodian fields provided + InsufficientCustodianFields, + /// Fields not provided for the application + FieldsNotProvided } #[pallet::call] @@ -435,6 +439,7 @@ pub mod pallet { ) -> DispatchResult { let who = ensure_signed(origin)?; + Self::validate_fields(&fields, &custodian_fields)?; let (custodian, fields) = Self::set_up_application(fields, custodian_fields); let application = Application:: { @@ -473,6 +478,7 @@ pub mod pallet { ) -> DispatchResult { let who = ensure_signed(origin)?; + Self::validate_fields(&fields, &custodian_fields)?; let (custodian, fields) = Self::set_up_application(fields, custodian_fields); let application = Application:: { diff --git a/pallets/gated-marketplace/src/mock.rs b/pallets/gated-marketplace/src/mock.rs index cf2eaeab..57ef15d5 100644 --- a/pallets/gated-marketplace/src/mock.rs +++ b/pallets/gated-marketplace/src/mock.rs @@ -163,16 +163,17 @@ impl pallet_balances::Config for Test { } parameter_types! { - pub const MaxScopesPerPallet: u32 = 2; - pub const MaxRolesPerPallet: u32 = 6; - pub const RoleMaxLen: u32 = 25; - pub const PermissionMaxLen: u32 = 25; - pub const MaxPermissionsPerRole: u32 = 30; - pub const MaxRolesPerUser: u32 = 2; - pub const MaxUsersPerRole: u32 = 2; + pub const MaxScopesPerPallet: u32 = 2; + pub const MaxRolesPerPallet: u32 = 6; + pub const RoleMaxLen: u32 = 25; + pub const PermissionMaxLen: u32 = 25; + pub const MaxPermissionsPerRole: u32 = 30; + pub const MaxRolesPerUser: u32 = 2; + pub const MaxUsersPerRole: u32 = 2; } impl pallet_rbac::Config for Test { type RuntimeEvent = RuntimeEvent; + type RemoveOrigin = EnsureRoot; type MaxScopesPerPallet = MaxScopesPerPallet; type MaxRolesPerPallet = MaxRolesPerPallet; type RoleMaxLen = RoleMaxLen; @@ -180,7 +181,7 @@ impl pallet_rbac::Config for Test { type MaxPermissionsPerRole = MaxPermissionsPerRole; type MaxRolesPerUser = MaxRolesPerUser; type MaxUsersPerRole = MaxUsersPerRole; - type RemoveOrigin = EnsureRoot; + type WeightInfo = (); } // Build genesis storage according to the mock runtime. From e89fe523538dae9996be3fd36aa916ad80d4e905 Mon Sep 17 00:00:00 2001 From: didiermis Date: Mon, 18 Mar 2024 17:17:21 -0600 Subject: [PATCH 2/7] - Corrected the condition in the `validate_fields` helper function in the Gated Marketplace pallet. Changed the condition to trigger the `FieldsNotProvided` error when `fields` are empty and `custodian_fields` are None, ensuring that the fields are provided when necessary. - Performed a minor clean-up in mock.rs, removing an unnecessary import of `RawOrigin` from the `frame_system` crate. This change promotes better code hygiene and optimizes imports. --- pallets/gated-marketplace/src/functions.rs | 2 +- pallets/gated-marketplace/src/mock.rs | 2 +- pallets/gated-marketplace/src/tests.rs | 1 + 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/pallets/gated-marketplace/src/functions.rs b/pallets/gated-marketplace/src/functions.rs index e3f57a80..870f51fe 100644 --- a/pallets/gated-marketplace/src/functions.rs +++ b/pallets/gated-marketplace/src/functions.rs @@ -806,7 +806,7 @@ impl Pallet { } } None => { - if !fields.is_empty() { + if fields.is_empty() { return Err(Error::::FieldsNotProvided.into()) } } diff --git a/pallets/gated-marketplace/src/mock.rs b/pallets/gated-marketplace/src/mock.rs index 57ef15d5..00125cc0 100644 --- a/pallets/gated-marketplace/src/mock.rs +++ b/pallets/gated-marketplace/src/mock.rs @@ -1,5 +1,4 @@ use crate as pallet_gated_marketplace; -use frame_system::RawOrigin; use sp_runtime::traits::Lookup; use frame_support::{ @@ -22,6 +21,7 @@ use sp_runtime::{ use system::EnsureSigned; type AccountId = u64; type AssetId = u32; + // Configure a mock runtime to test the pallet. frame_support::construct_runtime!( pub enum Test diff --git a/pallets/gated-marketplace/src/tests.rs b/pallets/gated-marketplace/src/tests.rs index 1c81196b..b0b0ffb3 100644 --- a/pallets/gated-marketplace/src/tests.rs +++ b/pallets/gated-marketplace/src/tests.rs @@ -378,6 +378,7 @@ fn apply_twice_shouldnt_work() { 1, )); let m_id = get_marketplace_id("my marketplace", 500, 600, 1); + assert_ok!(GatedMarketplace::apply( RuntimeOrigin::signed(3), m_id, From 34bfd70d2616af2c454e1188d2f49db57f341ebe Mon Sep 17 00:00:00 2001 From: didiermis Date: Tue, 19 Mar 2024 12:12:59 -0600 Subject: [PATCH 3/7] - Overhauled the `set_up_application` function in the Gated Marketplace pallet. Removed the separate `validate_fields` function and integrated its validation logic directly into `set_up_application`, providing field validation in the same context as application setup. This resulted in more streamlined validation, ensuring a non-empty fields list and correct matching of fields and custodian fields sizes directly in the application setup. Added error handling for the case that the number of application fields exceeds the maximum limit. - Adjusted the Gated Marketplace pallet structure and error definitions. Added the `ExceedMaxFilesApplication` error to handle scenarios where the number of application files surpasses the allowable maximum. This new error is now part of the error returns in the `enroll_application` and `enroll_self_managed_application` functions, improving the robustness and user feedback of our enrollment operations. --- pallets/gated-marketplace/src/functions.rs | 35 ++++++++-------------- pallets/gated-marketplace/src/lib.rs | 10 +++---- 2 files changed, 17 insertions(+), 28 deletions(-) diff --git a/pallets/gated-marketplace/src/functions.rs b/pallets/gated-marketplace/src/functions.rs index 870f51fe..3d8ee6ba 100644 --- a/pallets/gated-marketplace/src/functions.rs +++ b/pallets/gated-marketplace/src/functions.rs @@ -135,8 +135,7 @@ impl Pallet { // ensure the origin is owner or admin Self::is_authorized(authority.clone(), &marketplace_id, Permission::Enroll)?; - Self::validate_fields(&fields, &custodian_fields)?; - let (custodian, fields) = Self::set_up_application(fields, custodian_fields); + let (custodian, fields) = Self::set_up_application(fields, custodian_fields)?; let application = Application:: { status: ApplicationStatus::default(), @@ -795,29 +794,12 @@ impl Pallet { } /* ---- Helper functions ---- */ - pub fn validate_fields( - fields: &Fields, - custodian_fields: &Option>, - ) -> DispatchResult { - match custodian_fields { - Some(c_fields) => { - if fields.len() != c_fields.1.len() { - return Err(Error::::InsufficientCustodianFields.into()) - } - } - None => { - if fields.is_empty() { - return Err(Error::::FieldsNotProvided.into()) - } - } - } - Ok(()) - } - pub fn set_up_application( fields: Fields, custodian_fields: Option>, - ) -> (Option, BoundedVec) { + ) -> Result<(Option, BoundedVec), DispatchError> { + ensure!(!fields.is_empty(), Error::::FieldsNotProvided); + let mut f: Vec = fields .iter() .map(|tuple| ApplicationField { @@ -826,8 +808,12 @@ impl Pallet { custodian_cid: None, }) .collect(); + let custodian = match custodian_fields { Some(c_fields) => { + if fields.len() != c_fields.1.len() { + return Err(Error::::InsufficientCustodianFields.into()) + } for (i, field) in f.iter_mut().enumerate() { field.custodian_cid = Some(c_fields.1[i].clone()); } @@ -835,7 +821,10 @@ impl Pallet { }, _ => None, }; - (custodian, BoundedVec::::try_from(f).unwrap_or_default()) + + let fields = BoundedVec::::try_from(f). + map_err(|_| Error::::ExceedMaxFilesApplication)?; + Ok((custodian, fields)) } fn insert_in_auth_market_lists( diff --git a/pallets/gated-marketplace/src/lib.rs b/pallets/gated-marketplace/src/lib.rs index e6c55d85..79ba685a 100644 --- a/pallets/gated-marketplace/src/lib.rs +++ b/pallets/gated-marketplace/src/lib.rs @@ -341,7 +341,9 @@ pub mod pallet { /// Not enough custodian fields provided InsufficientCustodianFields, /// Fields not provided for the application - FieldsNotProvided + FieldsNotProvided, + /// Exceeds the maximum number of files + ExceedMaxFilesApplication, } #[pallet::call] @@ -439,8 +441,7 @@ pub mod pallet { ) -> DispatchResult { let who = ensure_signed(origin)?; - Self::validate_fields(&fields, &custodian_fields)?; - let (custodian, fields) = Self::set_up_application(fields, custodian_fields); + let (custodian, fields) = Self::set_up_application(fields, custodian_fields)?; let application = Application:: { status: ApplicationStatus::default(), @@ -478,8 +479,7 @@ pub mod pallet { ) -> DispatchResult { let who = ensure_signed(origin)?; - Self::validate_fields(&fields, &custodian_fields)?; - let (custodian, fields) = Self::set_up_application(fields, custodian_fields); + let (custodian, fields) = Self::set_up_application(fields, custodian_fields)?; let application = Application:: { status: ApplicationStatus::default(), From 3d96012d3f36a62255c1d058b0819a438f30edc1 Mon Sep 17 00:00:00 2001 From: didiermis Date: Tue, 19 Mar 2024 12:13:40 -0600 Subject: [PATCH 4/7] ntroduced two new test cases in the Gated Marketplace pallet to validate the functionality and error handling of the `apply` function. The test `apply_with_mismatched_number_of_files_and_custodian_files_shouldnt_work` ensures the function returns an error when the number of files and custodian files provided do not match. Similarly, the `apply_with_custodian_but_no_custodian_files_shouldnt_work` test verifies that `apply` correctly rejects applications where a custodian is specified but no files are provided. These tests improve the overall coverage and robustness of our testing for the Gated Marketplace pallet. --- pallets/gated-marketplace/src/tests.rs | 56 +++++++++++++++++++++++++- 1 file changed, 55 insertions(+), 1 deletion(-) diff --git a/pallets/gated-marketplace/src/tests.rs b/pallets/gated-marketplace/src/tests.rs index b0b0ffb3..35d9e0b6 100644 --- a/pallets/gated-marketplace/src/tests.rs +++ b/pallets/gated-marketplace/src/tests.rs @@ -273,6 +273,60 @@ fn apply_with_custodian_works() { }); } +#[test] +fn apply_with_mismatched_number_of_files_and_custodian_files_shouldnt_work() { + new_test_ext().execute_with(|| { + Balances::make_free_balance_be(&1, 100); + // Dispatch a signed extrinsic. + let m_label = create_label("my marketplace"); + assert_ok!(GatedMarketplace::create_marketplace( + RuntimeOrigin::signed(1), + 2, + m_label.clone(), + 500, + 600, + 1, + )); + let m_id = get_marketplace_id("my marketplace", 500, 600, 1); + assert_noop!( + GatedMarketplace::apply( + RuntimeOrigin::signed(3), + m_id, + create_application_fields(2), + create_custodian_fields(4, 1) + ), + Error::::InsufficientCustodianFields + ); + }); +} + +#[test] +fn apply_with_custodian_but_no_custodian_files_shouldnt_work() { + new_test_ext().execute_with(|| { + Balances::make_free_balance_be(&1, 100); + // Dispatch a signed extrinsic. + let m_label = create_label("my marketplace"); + assert_ok!(GatedMarketplace::create_marketplace( + RuntimeOrigin::signed(1), + 2, + m_label.clone(), + 500, + 600, + 1, + )); + let m_id = get_marketplace_id("my marketplace", 500, 600, 1); + assert_noop!( + GatedMarketplace::apply( + RuntimeOrigin::signed(3), + m_id, + create_application_fields(2), + create_custodian_fields(4, 0) + ), + Error::::InsufficientCustodianFields + ); + }); +} + #[test] fn apply_with_same_account_as_custodian_shouldnt_work() { new_test_ext().execute_with(|| { @@ -378,7 +432,7 @@ fn apply_twice_shouldnt_work() { 1, )); let m_id = get_marketplace_id("my marketplace", 500, 600, 1); - + assert_ok!(GatedMarketplace::apply( RuntimeOrigin::signed(3), m_id, From 0b8e196b77980c39cd12a8cb29be8378dfa5dd32 Mon Sep 17 00:00:00 2001 From: didiermis Date: Tue, 19 Mar 2024 12:34:47 -0600 Subject: [PATCH 5/7] Added a new test case, `apply_to_marketplace_without_fields_shouldnt_work`, to the Gated Marketplace pallet. This test verifies that an application without any fields leads to the `FieldsNotProvided` error. It enhances the test coverage for the `apply` function, ensuring it robustly catches instance where application fields are not provided as intended. --- pallets/gated-marketplace/src/tests.rs | 27 ++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/pallets/gated-marketplace/src/tests.rs b/pallets/gated-marketplace/src/tests.rs index 35d9e0b6..ce4e6c49 100644 --- a/pallets/gated-marketplace/src/tests.rs +++ b/pallets/gated-marketplace/src/tests.rs @@ -244,6 +244,33 @@ fn apply_to_marketplace_works() { }); } +#[test] +fn apply_to_marketplace_without_fields_shouldnt_work() { + new_test_ext().execute_with(|| { + Balances::make_free_balance_be(&1, 100); + // Dispatch a signed extrinsic. + let m_label = create_label("my marketplace"); + assert_ok!(GatedMarketplace::create_marketplace( + RuntimeOrigin::signed(1), + 2, + m_label.clone(), + 500, + 600, + 1, + )); + let m_id = get_marketplace_id("my marketplace", 500, 600, 1); + assert_noop!( + GatedMarketplace::apply( + RuntimeOrigin::signed(3), + m_id, + create_application_fields(0), + None + ), + Error::::FieldsNotProvided + ); + }); +} + #[test] fn apply_with_custodian_works() { new_test_ext().execute_with(|| { From a92088c1d41bb859cdc21f9635712a0c2796a780 Mon Sep 17 00:00:00 2001 From: didiermis Date: Tue, 19 Mar 2024 15:28:26 -0600 Subject: [PATCH 6/7] Updated test name --- pallets/gated-marketplace/src/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pallets/gated-marketplace/src/tests.rs b/pallets/gated-marketplace/src/tests.rs index ce4e6c49..eabb527f 100644 --- a/pallets/gated-marketplace/src/tests.rs +++ b/pallets/gated-marketplace/src/tests.rs @@ -328,7 +328,7 @@ fn apply_with_mismatched_number_of_files_and_custodian_files_shouldnt_work() { } #[test] -fn apply_with_custodian_but_no_custodian_files_shouldnt_work() { +fn apply_with_custodian_but_no_custodian_fields_shouldnt_work() { new_test_ext().execute_with(|| { Balances::make_free_balance_be(&1, 100); // Dispatch a signed extrinsic. From ae9340d0ac83185f41de011115929dfcb0e5c6f6 Mon Sep 17 00:00:00 2001 From: didiermis Date: Tue, 19 Mar 2024 16:07:23 -0600 Subject: [PATCH 7/7] Updated test name --- pallets/gated-marketplace/src/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pallets/gated-marketplace/src/tests.rs b/pallets/gated-marketplace/src/tests.rs index eabb527f..519481ea 100644 --- a/pallets/gated-marketplace/src/tests.rs +++ b/pallets/gated-marketplace/src/tests.rs @@ -301,7 +301,7 @@ fn apply_with_custodian_works() { } #[test] -fn apply_with_mismatched_number_of_files_and_custodian_files_shouldnt_work() { +fn apply_with_mismatched_number_of_fields_and_custodian_fields_shouldnt_work() { new_test_ext().execute_with(|| { Balances::make_free_balance_be(&1, 100); // Dispatch a signed extrinsic.