on_dispute_complete refactor, tests, test refactor

pallets/disputes/src/lib.rs

@@ -62,7 +62,7 @@ pub mod pallet {
         /// The origin used to force cancel and pass disputes.
         type ForceOrigin: EnsureOrigin<Self::RuntimeOrigin>;
         /// External hooks to handle the completion of a dispute.
-        type DisputeHooks: DisputeHooks<Self::DisputeKey, Self::SpecificId>;
+        type DisputeHooks: DisputeHooks<Self::DisputeKey, Self::SpecificId, AccountIdOf<Self>>;
     }
 
     /// Used to store the disputes that is being raised, given the dispute key it returns the Dispute
@@ -145,8 +145,9 @@ pub mod pallet {
                     let hook_weight = <T::DisputeHooks as DisputeHooks<
                         T::DisputeKey,
                         T::SpecificId,
+                        T::AccountId,
                     >>::on_dispute_complete(
-                        *dispute_id, dispute.specifiers.into_inner(), result
+                        dispute.raised_by, *dispute_id, dispute.specifiers.into_inner(), result
                     );
 
                     weight = weight.saturating_add(hook_weight);
@@ -216,6 +217,7 @@ pub mod pallet {
                 // Dont mind if this fails as the autofinalise will skip.
             });
             T::DisputeHooks::on_dispute_complete(
+                dispute.raised_by,
                 dispute_key,
                 dispute.specifiers.into_inner(),
                 DisputeResult::Failure,
@@ -248,6 +250,7 @@ pub mod pallet {
                 // Dont mind if this fails as the autofinalise will skip.
             });
             T::DisputeHooks::on_dispute_complete(
+                dispute.raised_by,
                 dispute_key,
                 dispute.specifiers.into_inner(),
                 DisputeResult::Success,
@@ -413,6 +416,7 @@ pub mod pallet {
 
             // Dont need to return the weight here.
             let _ = T::DisputeHooks::on_dispute_complete(
+                dispute.raised_by,
                 dispute_key,
                 dispute.specifiers.into_inner(),
                 result,

pallets/disputes/src/traits.rs

@@ -18,10 +18,11 @@ pub trait DisputeRaiser<AccountId> {
     ) -> Result<(), DispatchError>;
 }
 
-pub trait DisputeHooks<DisputeKey, SpecificId> {
+pub trait DisputeHooks<DisputeKey, SpecificId, AccountId> {
     /// On the completion of a dispute, this hooks is called.
     /// Returning only the key that has been handled and the result of the dispute.
     fn on_dispute_complete(
+        raised_by: AccountId,
         dispute_key: DisputeKey,
         specifics: Vec<SpecificId>,
         dispute_result: crate::pallet::DisputeResult,

pallets/proposals/src/impls/pallet_impls.rs

@@ -337,8 +337,9 @@ impl<T: Config> Pallet<T> {
     }
 }
 
-impl<T: Config> DisputeHooks<ProjectKey, MilestoneKey> for Pallet<T> {
+impl<T: Config> DisputeHooks<ProjectKey, MilestoneKey, AccountIdOf<T>> for Pallet<T> {
     fn on_dispute_complete(
+        raised_by: AccountIdOf<T>,
         project_key: ProjectKey,
         specifics: Vec<MilestoneKey>,
         dispute_result: pallet_disputes::pallet::DisputeResult,
@@ -354,7 +355,11 @@ impl<T: Config> DisputeHooks<ProjectKey, MilestoneKey> for Pallet<T> {
                                     // Shouldnt be needed but nice to have this check.
                                     // Will prevent someone calling both refund and withdraw on the same milestone.
                                     if milestone.transfer_status.is_none() {
-                                        milestone.can_refund = true;
+                                        if project.initiator == raised_by {
+                                            milestone.is_approved = true;
+                                        } else {
+                                            milestone.can_refund = true;
+                                        }
                                     }
                                 }
                             }

pallets/proposals/src/lib.rs

@@ -344,8 +344,8 @@ pub mod pallet {
         TooManyMilestoneVotes,
         /// An internal error, a collection of votes for a milestone has been lost.s
         IndividualVoteNotFound,
-        /// Only a contributor can raise a dispute.
-        OnlyContributorsCanRaiseDispute,
+        /// Only a contributor can initiate a refund.
+        OnlyContributorsCanInitiateRefund,
         /// One of these milestones is already in a dispute.
         MilestonesAlreadyInDispute,
         /// You cannot raise a dispute on an approved milestone.
@@ -456,10 +456,10 @@ pub mod pallet {
                 Error::<T>::MilestoneDoesNotExist
             );
             ensure!(
-                project.contributions.contains_key(&who) || &who == project.initiator,
+                project.contributions.contains_key(&who) || &who == &project.initiator,
                 Error::<T>::NotPermittedToRaiseDispute
             );
-            
+
             ensure!(
                 !ProjectsInDispute::<T>::contains_key(project_key),
                 Error::<T>::MilestonesAlreadyInDispute
@@ -473,7 +473,7 @@ pub mod pallet {
 
             if project.jury.len() == 1 {
                 // https://github.com/ImbueNetwork/imbue/issues/270
-                let _ = <Self as pallet_disputes::traits::DisputeHooks<ProjectKey, MilestoneKey>>::on_dispute_complete(project_key, milestone_keys.to_vec(), pallet_disputes::DisputeResult::Success);
+                let _ = <Self as pallet_disputes::traits::DisputeHooks<ProjectKey, MilestoneKey, AccountIdOf<T>>>::on_dispute_complete(who, project_key, milestone_keys.to_vec(), pallet_disputes::DisputeResult::Success);
             } else {
                 <T as Config>::DisputeRaiser::raise_dispute(
                     project_key,

pallets/proposals/src/test_utils.rs

@@ -140,11 +140,12 @@ pub fn create_funded_user<T: Config>(
 
 /// Manually call the hook OnDisputeCompleteWith a predefined result for testing>
 pub fn complete_dispute<T: Config>(
+    dispute_raised_by: T::AccountId,
     project_key: ProjectKey,
     milestone_keys: Vec<MilestoneKey>,
     result: pallet_disputes::DisputeResult,
 ) -> crate::Weight {
-    <crate::Pallet<T>>::on_dispute_complete(project_key, milestone_keys, result)
+    <crate::Pallet<T>>::on_dispute_complete(dispute_raised_by, project_key, milestone_keys, result)
 }
 
 pub fn assert_last_event<T: Config>(generic_event: <T as Config>::RuntimeEvent) {

pallets/proposals/src/tests/dispute_approvals.rs

@@ -0,0 +1,54 @@
+use crate::{mock::*, *};
+use frame_support::{assert_noop, assert_ok};
+use pallet_disputes::DisputeResult;
+use test_utils::*;
+
+#[test]
+fn initiator_dispute_complete_sets_milestones_to_approved() {
+    build_test_externality().execute_with(|| {
+        let per_contribution = 100000u128;
+        let contributions = get_contributions::<Test>(vec![BOB, CHARLIE], per_contribution);
+        let milestones = get_milestones(10);
+        let jury = vec![JURY_1, JURY_2];
+        let initiator = ALICE;
+
+        let project_key = create_and_fund_project::<Test>(
+            ALICE,
+            contributions,
+            milestones.clone(),
+            CurrencyId::Native,
+            jury,
+        )
+        .unwrap();
+        let milestone_keys: BoundedVec<u32, <Test as Config>::MaxMilestonesPerProject> = (1u32
+            ..milestones.len() as u32)
+            .collect::<Vec<u32>>()
+            .try_into()
+            .unwrap();
+
+        assert_ok!(Proposals::raise_dispute(
+            RuntimeOrigin::signed(initiator),
+            project_key,
+            milestone_keys.clone()
+        ));
+
+        let _ = complete_dispute::<Test>(
+            initiator,
+            project_key,
+            milestone_keys.into_inner(),
+            DisputeResult::Success,
+        );
+
+        let project = Projects::<Test>::get(project_key).unwrap();
+
+        for milestone in project.milestones.iter().for_each(|(key, milestone)|{
+            if milestone_keys.contains(&key) {
+                assert!(milestone.is_approved, "dispute success for initiator should approve milestones.")
+            } else {
+                assert!(!milestone.is_approved, "other milestones should be left unapproved.")
+            }
+        })
+    })
+}
+
+

pallets/proposals/src/tests/disputes.rs

@@ -206,6 +206,7 @@ fn on_dispute_complete_success_removes_dispute_status() {
             milestone_keys.clone()
         ));
         let _ = complete_dispute::<Test>(
+            BOB,
             project_key,
             milestone_keys.into_inner(),
             DisputeResult::Success,
@@ -240,6 +241,7 @@ fn on_dispute_complete_failure_removes_dispute_status() {
             milestone_keys.clone()
         ));
         let _ = complete_dispute::<Test>(
+            BOB,
             project_key,
             milestone_keys.into_inner(),
             DisputeResult::Failure,
@@ -275,6 +277,7 @@ fn dispute_success_does_not_cancel_project() {
             milestone_keys.clone()
         ));
         let _ = complete_dispute::<Test>(
+            BOB,
             project_key,
             milestone_keys.into_inner(),
             DisputeResult::Success,
@@ -312,6 +315,7 @@ fn dispute_success_approves_milestone_for_refund_but_only_ones_specified() {
             milestone_keys.clone()
         ));
         let _ = complete_dispute::<Test>(
+            BOB,
             project_key,
             milestone_keys.into_inner(),
             DisputeResult::Success,
@@ -485,6 +489,7 @@ fn failed_dispute_tests() {
             dispute_milestone_keys.clone()
         ));
         let _ = complete_dispute::<Test>(
+            BOB,
             project_key,
             dispute_milestone_keys.into_inner(),
             DisputeResult::Failure,
@@ -525,6 +530,7 @@ fn assert_can_recall_dispute_after_success() {
             milestone_keys.clone()
         ));
         let _ = complete_dispute::<Test>(
+            BOB,
             project_key,
             milestone_keys.into_inner(),
             DisputeResult::Success,
@@ -564,6 +570,7 @@ fn assert_can_recall_dispute_after_failure() {
             milestone_keys.clone()
         ));
         let _ = complete_dispute::<Test>(
+            BOB,
             project_key,
             milestone_keys.into_inner(),
             DisputeResult::Failure,
@@ -633,7 +640,6 @@ fn raise_dispute_as_initiator_success() {
     build_test_externality().execute_with(|| {
         let contributions = get_contributions::<Test>(vec![BOB, CHARLIE], 1_000_000u128);
         let milestones = get_milestones(10);
-        let milestone_key = 0;
         let jury = vec![JURY_1, JURY_2];
 
         let project_key = create_and_fund_project::<Test>(
@@ -663,7 +669,6 @@ fn raise_dispute_as_contributor_success() {
     build_test_externality().execute_with(|| {
         let contributions = get_contributions::<Test>(vec![BOB, CHARLIE], 1_000_000u128);
         let milestones = get_milestones(10);
-        let milestone_key = 0;
         let jury = vec![JURY_1, JURY_2];
 
         let project_key = create_and_fund_project::<Test>(

pallets/proposals/src/tests/refunds.rs

@@ -29,6 +29,7 @@ fn you_can_actually_refund_after_dispute_success() {
             milestone_keys.clone()
         ));
         let _ = complete_dispute::<Test>(
+            BOB,
             project_key,
             milestone_keys.into_inner(),
             DisputeResult::Success,
@@ -63,6 +64,7 @@ fn refund_assert_milestone_state_change() {
             milestone_keys.clone()
         ));
         let _ = complete_dispute::<Test>(
+            BOB,
             project_key,
             milestone_keys.into_inner(),
             DisputeResult::Success,
@@ -116,6 +118,7 @@ fn refund_not_contributor() {
             milestone_keys.clone()
         ));
         let _ = complete_dispute::<Test>(
+            BOB,
             project_key,
             milestone_keys.into_inner(),
             DisputeResult::Success,
@@ -153,6 +156,7 @@ fn refund_deletes_project_when_all_funds_are_refunded() {
             milestone_keys.clone()
         ));
         let _ = complete_dispute::<Test>(
+            BOB,
             project_key,
             milestone_keys.into_inner(),
             DisputeResult::Success,
@@ -198,6 +202,7 @@ fn withdraw_then_refund_no_double_spend() {
             milestone_keys.clone(),
         );
         let _ = complete_dispute::<Test>(
+            BOB,
             project_key,
             milestone_keys.into_inner(),
             DisputeResult::Success,
@@ -268,6 +273,7 @@ fn refund_then_withdraw_no_double_spend() {
             milestone_keys.clone(),
         );
         let _ = complete_dispute::<Test>(
+            BOB,
             project_key,
             milestone_keys.into_inner(),
             DisputeResult::Success,
@@ -322,6 +328,7 @@ fn refund_check_refund_amount() {
             milestone_keys.clone()
         ));
         let _ = complete_dispute::<Test>(
+            BOB,
             project_key,
             milestone_keys.into_inner(),
             DisputeResult::Success,
@@ -383,6 +390,7 @@ fn refund_takes_imbue_fee() {
             milestone_keys.clone()
         ));
         let _ = complete_dispute::<Test>(
+            BOB,
             project_key,
             milestone_keys.into_inner(),
             DisputeResult::Success,
@@ -419,3 +427,4 @@ fn refund_takes_imbue_fee() {
         );
     })
 }
+