Out of bounds marketplace application #41

didiermis · 2024-03-18T23:23:27Z

Validation Fixes and Tests in Gated Marketplace

Overview

The changes implemented focus on enhancing the gated marketplace pallet by introducing error handling for application setup and enforcing validation checks. The motivation behind these changes is to ensure that applications to the marketplace meet specific criteria, such as having the required fields and adhering to the constraints for custodian relationships. This improvement in validation prevents potential runtime errors and enhances the overall robustness of the marketplace application process.

Tickets

Fix [High] Reachable panic via index out of bounds in gated-marketplace pallet #29

Implementation notes

The implementation involves modifying the set_up_application function to return a Result type, enabling the function to handle errors gracefully by leveraging Rust's error handling paradigm. Additional checks are introduced to ensure that:

The fields provided for an application are not empty.
The number of custodian fields matches the number of application fields when custodians are involved.
The maximum number of files specified for an application is not exceeded.

These checks are accomplished using the ensure! macro for concise assertions and the .map_err method to transform errors from the try_from method into pallet-specific errors.

Furthermore, corresponding changes are made in the callers of set_up_application within the module to handle the Result returned by the

Interesting/controversial decisions

The decision to enforce strict validation at the application setup stage could be seen as restrictive, especially in scenarios where marketplace dynamics might demand more flexibility.

Test coverage

New tests were added in tests.rs to cover scenarios involving the new validations. These include tests for applying without fields, applying with mismatched numbers of fields and custodian fields, and exceeding the maximum allowed fields. The tests verify that the system correctly rejects invalid or incomplete applications, and they help ensure that the validations work as intended.

…ketplace 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.

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

github-actions · 2024-03-18T23:23:43Z

pallets/gated-marketplace/src/mock.rs

@@ -1,5 +1,4 @@
 use crate as pallet_gated_marketplace;


GPT summary of cf2eae - 00125c:
Error: couldn't generate summary

github-actions · 2024-03-18T23:23:46Z

GPT summary of e97d36e:

Error: couldn't generate summary

github-actions · 2024-03-18T23:23:48Z

GPT summary of e89fe52:

Error: couldn't generate summary

PR summary so far:

Error: couldn't generate summary

sebastianmontero

Para los PRs que arreglan un issue, primero deberiamos de desarrollar un prueba que descubre el issue, y después desarrollar el codigo que lo arregla, para asegurarnos de que el issue esta arreglado.
Por lo que veo tambien faltan pruebas para el caso en el que el custodian fields es None, ya que este es un caso valido, y el nuevo metodo de validate_fields lanza un error en este caso, sin embargo todas las pruebas pasan.

sebastianmontero · 2024-03-19T00:24:29Z

pallets/gated-marketplace/src/functions.rs

+			}
+			None => {
+				if fields.is_empty() {
+					return Err(Error::<T>::FieldsNotProvided.into())


Por que se manda un error cuando el campo es None? Es un caso valido el no tener custodian fields.

En el caso donde custodian_fields es None, está cubierto en el test apply_to_marketplace_works

assert_ok!(GatedMarketplace::apply( RuntimeOrigin::signed(3), m_id, create_application_fields(2), None ));

Eliminé la función validate_fields y moví la lógica a la función set_up_application debido a que cambié el tipo de retorno de ésta última. La razón inicial de separarlo fue que una devolvía un dipatchresult y la otra una tupla.

sebastianmontero · 2024-03-19T00:27:52Z

pallets/gated-marketplace/src/lib.rs

 		) -> DispatchResult {
 			let who = ensure_signed(origin)?;

+			Self::validate_fields(&fields, &custodian_fields)?;


creo que el metodo validate_fields se deberia de llamar dentro de set_up_application, que es donde se encuentra el codigo que causa el issue

Movi la validación hacia set_up_application y también hice un update en el tipo de retorno para dicha función.

…ce 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.

…ate 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.

github-actions · 2024-03-19T18:14:13Z

pallets/gated-marketplace/src/functions.rs

 			!<ApplicationsByAccount<T>>::contains_key(new_user.clone(), marketplace_id),
 			Error::<T>::AlreadyApplied
 		);
 		// ensure the origin is owner or admin


GPT summary of f3ad44 - 3d8ee6:
Error: couldn't generate summary

github-actions · 2024-03-19T18:14:14Z

pallets/gated-marketplace/src/lib.rs

 		/// User is already blocked
 		UserAlreadyBlocked,
 		/// The owner of the NFT is not in the marketplace
 		OwnerNotInMarketplace,


GPT summary of edca59 - 79ba68:
Error: couldn't generate summary

github-actions · 2024-03-19T18:14:19Z

GPT summary of 34bfd70:

Error: couldn't generate summary

github-actions · 2024-03-19T18:14:20Z

GPT summary of 3d96012:

Error: couldn't generate summary

PR summary so far:

Error: couldn't generate summary

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

github-actions · 2024-03-19T18:35:07Z

GPT summary of 0b8e196:

Error: couldn't generate summary

PR summary so far:

Error: couldn't generate summary

sebastianmontero · 2024-03-19T19:38:35Z

pallets/gated-marketplace/src/tests.rs

 }

+#[test]
+fn apply_with_mismatched_number_of_files_and_custodian_files_shouldnt_work() {


Nada mas para mantener la consistencia cambiaria "files" por "fields" en el nombre de la prueba

Falta cambiar el nombre de esta prueba

sebastianmontero · 2024-03-19T19:38:49Z

pallets/gated-marketplace/src/tests.rs

+}
+
+#[test]
+fn apply_with_custodian_but_no_custodian_files_shouldnt_work() {


Nada mas para mantener la consistencia cambiaria "files" por "fields" en el nombre de la prueba

github-actions · 2024-03-19T21:28:55Z

GPT summary of a92088c:

Error: couldn't generate summary

PR summary so far:

Error: couldn't generate summary

sebastianmontero · 2024-03-19T21:59:13Z

pallets/gated-marketplace/src/tests.rs

 }

+#[test]
+fn apply_with_mismatched_number_of_files_and_custodian_files_shouldnt_work() {


Falta cambiar el nombre de esta prueba

github-actions · 2024-03-19T22:07:44Z

pallets/gated-marketplace/src/tests.rs

 			GatedMarketplace::applicants_by_marketplace(m_id, ApplicationStatus::Pending).len() ==
 				1
 		);
 	});


GPT summary of 1c8119 - 519481:
Error: couldn't generate summary

github-actions · 2024-03-19T22:07:46Z

GPT summary of ae9340d:

Error: couldn't generate summary

PR summary so far:

Error: couldn't generate summary

didiermis added 2 commits March 18, 2024 15:14

didiermis requested review from sebastianmontero and tlacloc March 18, 2024 23:23

didiermis self-assigned this Mar 18, 2024

github-actions bot reviewed Mar 18, 2024

View reviewed changes

didiermis changed the title ~~Feature/benchmark/ticket/29~~ Out of bounds marketplace application Mar 18, 2024

sebastianmontero requested changes Mar 19, 2024

View reviewed changes

didiermis added 2 commits March 19, 2024 12:12

github-actions bot reviewed Mar 19, 2024

View reviewed changes

sebastianmontero requested changes Mar 19, 2024

View reviewed changes

Updated test name

a92088c

sebastianmontero requested changes Mar 19, 2024

View reviewed changes

Updated test name

ae9340d

github-actions bot reviewed Mar 19, 2024

View reviewed changes

didiermis requested review from sebastianmontero and removed request for tlacloc March 19, 2024 22:08

sebastianmontero approved these changes Mar 19, 2024

View reviewed changes

sebastianmontero merged commit 37de4d9 into feature/benchmarks Mar 19, 2024

Out of bounds marketplace application #41

Out of bounds marketplace application #41

Uh oh!

Conversation

didiermis commented Mar 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Validation Fixes and Tests in Gated Marketplace

Overview

Tickets

Implementation notes

Interesting/controversial decisions

Test coverage

Uh oh!

github-actions bot Mar 18, 2024

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 18, 2024

Uh oh!

github-actions bot commented Mar 18, 2024

Uh oh!

sebastianmontero left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

didiermis Mar 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

didiermis Mar 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 19, 2024

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 19, 2024

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 19, 2024

Uh oh!

github-actions bot commented Mar 19, 2024

Uh oh!

github-actions bot commented Mar 19, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 19, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 19, 2024

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

didiermis commented Mar 18, 2024 •

edited

Loading

didiermis Mar 19, 2024 •

edited

Loading

didiermis Mar 19, 2024 •

edited

Loading