Skip to content

Conversation

@lrazovic
Copy link
Member

@lrazovic lrazovic commented May 12, 2025

This pull request refactors the EvaluationInfo structure and associated logic across the pallets/funding module to simplify its design by removing the id and project_id fields from the structure. The changes also update related functions, storage, and tests to accommodate this refactoring. Additionally, some minor cleanups were performed.

Refactoring of EvaluationInfo and Related Logic:

  • EvaluationInfo Structure Simplification:

    • Removed the id and project_id fields from the EvaluationInfo struct definition in pallets/funding/src/types.rs. The struct now relies on external identifiers for these fields.
    • Updated the associated type alias EvaluationInfoOf<T> in pallets/funding/src/lib.rs to reflect the new structure.
  • Function Updates:

    • Updated do_settle_evaluation in pallets/funding/src/functions/5_settlement.rs to accept evaluation_id as a separate parameter instead of using it from the EvaluationInfo struct. [1] [2]
    • Adjusted try_plmc_participation_lock to iterate over evaluation IDs and include them in updates to the Evaluations storage.
    • Modified settle_project and get_evaluations in pallets/funding/src/instantiator/chain_interactions.rs to handle the new structure. [1] [2]
  • Storage Updates:

    • Updated all references to Evaluations storage to use external evaluation_id where necessary, ensuring compatibility with the new structure. [1] [2]

Test Adjustments:

  • Test Updates for EvaluationInfo Changes:
    • Updated tests in pallets/funding/src/tests/5_settlement.rs to reflect the new EvaluationInfo structure by using tuples (AccountId, EvaluationId, EvaluationInfo) instead of the old structure. [1] [2] [3]
    • Adjusted assertions and test logic to handle the separation of evaluation_id from EvaluationInfo. [1] [2]

Minor Cleanups:

  • Removed unused id and project_id fields from EvaluationInfo in pallets/funding/src/functions/2_evaluation.rs and associated logic. [1] [2]
  • Removed unnecessary #[allow(unreachable_patterns)] directive in pallets/funding/src/lib.rs.
  • Added BlockNumberFor import to pallets/funding/src/lib.rs for consistency.

These changes streamline the EvaluationInfo structure and improve the maintainability of the codebase by reducing redundancy and simplifying related logic.

Copy link
Member Author

lrazovic commented May 12, 2025

@lrazovic lrazovic force-pushed the 05-12-feat_remove_account_and_id_from_stored_value branch 2 times, most recently from 0ed680d to 9c7636a Compare May 13, 2025 07:17
@lrazovic lrazovic changed the title feat: remove account and id from stored value ⚡️ Remove AccountId and id from stored value May 16, 2025
@lrazovic lrazovic requested a review from Copilot May 16, 2025 14:43
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR removes the stored fields “id” and “project_id” from the EvaluationInfo struct and updates all related calls and tests accordingly. Key changes include:

  • Updating the EvaluationInfo struct in pallets/funding/src/types.rs.
  • Adjusting tests in pallets/funding/src/tests/5_settlement.rs and pallets/funding/src/tests/2_evaluation.rs to account for the changed struct.
  • Modifying helper functions and instantiators to work with a tuple structure returned by get_evaluations.

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
pallets/funding/src/types.rs Removed the id and project_id fields from EvaluationInfo.
pallets/funding/src/tests/5_settlement.rs Updated tuple deconstruction and other references in tests.
pallets/funding/src/tests/2_evaluation.rs Adjusted expected EvaluationInfo to match struct changes.
pallets/funding/src/lib.rs Updated type alias and calls for EvaluationInfo.
pallets/funding/src/instantiator/chain_interactions.rs Modified get_evaluations to return tuples instead of raw values.
pallets/funding/src/functions/misc.rs Updated evaluation locking logic with tuple access.
pallets/funding/src/functions/5_settlement.rs Changed do_settle_evaluation signature to include evaluation_id.
pallets/funding/src/functions/2_evaluation.rs Updated EvaluationInfo instantiation to remove obsolete fields.

Comment on lines 437 to +451
let first_evaluation = inst.get_evaluations(project_id).into_iter().next().unwrap();
inst.execute(|| {
let evaluator = first_evaluation.evaluator;
let evaluator = first_evaluation.0;
assert_ok!(crate::Pallet::<TestRuntime>::settle_evaluation(
RuntimeOrigin::signed(evaluator),
project_id,
evaluator,
first_evaluation.id
first_evaluation.1
));
assert_noop!(
crate::Pallet::<TestRuntime>::settle_evaluation(
RuntimeOrigin::signed(evaluator),
project_id,
evaluator,
first_evaluation.id
first_evaluation.1
Copy link

Copilot AI May 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Consider destructuring the tuple returned by get_evaluations instead of accessing its elements directly via indices (e.g. first_evaluation.0 and first_evaluation.1) to improve code readability.

Copilot uses AI. Check for mistakes.

pub fn get_evaluations(&mut self, project_id: ProjectId) -> Vec<EvaluationInfoOf<T>> {
self.execute(|| Evaluations::<T>::iter_prefix_values((project_id,)).collect())
pub fn get_evaluations(&mut self, project_id: ProjectId) -> Vec<(AccountIdOf<T>, u32, EvaluationInfoOf<T>)> {
Copy link

Copilot AI May 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The revised tuple structure returned by get_evaluations requires careful handling; ensure that all call sites properly destructure and use the tuple values.

Copilot uses AI. Check for mistakes.
@lrazovic lrazovic requested a review from dastansam May 16, 2025 14:45
@lrazovic lrazovic self-assigned this May 16, 2025
@lrazovic lrazovic marked this pull request as ready for review May 16, 2025 14:46
@lrazovic lrazovic force-pushed the 05-08-draft_use_plmc_amount_in_evaluation branch from fce9db1 to f9cceec Compare May 19, 2025 06:15
@lrazovic lrazovic force-pushed the 05-12-feat_remove_account_and_id_from_stored_value branch 2 times, most recently from 67b6aa1 to a998aff Compare May 19, 2025 06:48
@lrazovic lrazovic force-pushed the 05-08-draft_use_plmc_amount_in_evaluation branch from f9cceec to 2ffd8f6 Compare May 19, 2025 06:48
@lrazovic lrazovic force-pushed the 05-12-feat_remove_account_and_id_from_stored_value branch from a998aff to 0d7c547 Compare May 20, 2025 06:21
@lrazovic lrazovic force-pushed the 05-08-draft_use_plmc_amount_in_evaluation branch from 2ffd8f6 to 0c2cd35 Compare May 20, 2025 06:21
@dastansam dastansam force-pushed the 05-12-feat_remove_account_and_id_from_stored_value branch from 0d7c547 to 992689e Compare May 23, 2025 11:43
@dastansam dastansam force-pushed the 05-08-draft_use_plmc_amount_in_evaluation branch from 0c2cd35 to 8c809b8 Compare May 23, 2025 11:43
@dastansam dastansam mentioned this pull request May 23, 2025
@lrazovic lrazovic force-pushed the 05-12-feat_remove_account_and_id_from_stored_value branch from 992689e to aa8e672 Compare May 28, 2025 13:35
@lrazovic lrazovic force-pushed the 05-08-draft_use_plmc_amount_in_evaluation branch 2 times, most recently from 7714174 to 909a092 Compare May 28, 2025 13:38
@lrazovic lrazovic force-pushed the 05-12-feat_remove_account_and_id_from_stored_value branch from aa8e672 to 7fcb836 Compare May 28, 2025 13:38
@dastansam dastansam force-pushed the 05-08-draft_use_plmc_amount_in_evaluation branch from 909a092 to ac45b29 Compare July 9, 2025 06:38
@dastansam dastansam force-pushed the 05-12-feat_remove_account_and_id_from_stored_value branch from 7fcb836 to 33fe2fd Compare July 9, 2025 06:38
@lrazovic lrazovic force-pushed the 05-12-feat_remove_account_and_id_from_stored_value branch from 33fe2fd to 4783aa9 Compare July 25, 2025 14:19
@lrazovic lrazovic force-pushed the 05-08-draft_use_plmc_amount_in_evaluation branch from ac45b29 to 5559689 Compare July 25, 2025 14:19
@dastansam dastansam force-pushed the 05-08-draft_use_plmc_amount_in_evaluation branch from 5559689 to ac45b29 Compare July 25, 2025 21:21
@dastansam dastansam force-pushed the 05-12-feat_remove_account_and_id_from_stored_value branch from 4783aa9 to 33fe2fd Compare July 25, 2025 21:21
@lrazovic lrazovic force-pushed the 05-08-draft_use_plmc_amount_in_evaluation branch 2 times, most recently from 7ba84ad to 8f206da Compare August 4, 2025 07:59
@lrazovic lrazovic force-pushed the 05-12-feat_remove_account_and_id_from_stored_value branch from 33fe2fd to 6daa42f Compare August 4, 2025 07:59
@lrazovic lrazovic closed this Aug 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants