Adjustments

pallets/dapps-staking/src/pallet/mod.rs

@@ -747,7 +747,7 @@ pub mod pallet {
             Self::ensure_pallet_enabled()?;
             let staker = ensure_signed(origin)?;
 
-            Self::internal_claim_staker_for(staker, contract_id)
+            Self::internal_claim_staker_for(staker.clone(), staker, contract_id, false)
         }
 
         /// Claim earned dapp rewards for the specified era.
@@ -930,22 +930,7 @@ pub mod pallet {
             Self::ensure_pallet_enabled()?;
             let caller = ensure_signed(origin)?;
 
-            let result = Self::internal_claim_staker_for(staker.clone(), contract_id)?;
-
-            let delegated_claim_fee = T::Currency::withdraw(
-                &staker,
-                T::DelegateClaimFee::get(),
-                WithdrawReasons::TRANSFER,
-                ExistenceRequirement::KeepAlive,
-            )?;
-            T::Currency::resolve_creating(&caller, delegated_claim_fee);
-
-            // Weight-wise these two ops are almost free since both accounts balances have already been read & written to.
-            // - caller: to pay for the transaction fee
-            // - staker: to deposit the reward
-            //
-            // Which means we don't need to add new benchmarks just for this (given the fact pallet will be obsolete soon)
-            Ok(result)
+            Self::internal_claim_staker_for(caller, staker, contract_id, true)
         }
 
         /// Used to set reward destination for staker rewards, for the given staker
@@ -1301,8 +1286,10 @@ pub mod pallet {
 
         /// Claim earned staker rewards for the given staker, and the oldest unclaimed era.
         fn internal_claim_staker_for(
+            caller: T::AccountId,
             staker: T::AccountId,
             contract_id: T::SmartContract,
+            refund_caller: bool,
         ) -> DispatchResultWithPostInfo {
             // Ensure we have something to claim
             let mut staker_info = Self::staker_info(&staker, &contract_id);
@@ -1325,7 +1312,7 @@ pub mod pallet {
 
             let (_, stakers_joint_reward) =
                 Self::dev_stakers_split(&staking_info, &reward_and_stake);
-            let staker_reward =
+            let max_staker_reward =
                 Perbill::from_rational(staked, staking_info.total) * stakers_joint_reward;
 
             let mut ledger = Self::ledger(&staker);
@@ -1338,7 +1325,7 @@ pub mod pallet {
 
             if should_restake_reward {
                 staker_info
-                    .stake(current_era, staker_reward)
+                    .stake(current_era, max_staker_reward)
                     .map_err(|_| Error::<T>::UnexpectedStakeInfoEra)?;
 
                 // Restaking will, in the worst case, remove one, and add one record,
@@ -1350,13 +1337,21 @@ pub mod pallet {
             }
 
             // Withdraw reward funds from the dapps staking pot
-            let reward_imbalance = T::Currency::withdraw(
+            let total_imbalance = T::Currency::withdraw(
                 &Self::account_id(),
-                staker_reward,
+                max_staker_reward,
                 WithdrawReasons::TRANSFER,
                 ExistenceRequirement::AllowDeath,
             )?;
 
+            let (refund_imbalance, reward_imbalance) = if refund_caller {
+                let claim_fee = T::DelegateClaimFee::get();
+                total_imbalance.split(claim_fee)
+            } else {
+                (NegativeImbalanceOf::<T>::zero(), total_imbalance)
+            };
+
+            let staker_reward = reward_imbalance.peek();
             if should_restake_reward {
                 ledger.locked = ledger.locked.saturating_add(staker_reward);
                 Self::update_ledger(&staker, ledger);
@@ -1384,8 +1379,17 @@ pub mod pallet {
 
             T::Currency::resolve_creating(&staker, reward_imbalance);
             Self::update_staker_info(&staker, &contract_id, staker_info);
+
             Self::deposit_event(Event::<T>::Reward(staker, contract_id, era, staker_reward));
 
+            // Weight-wise this operation is essentially free since it caller balance is already updated to due to the fee.
+            //
+            // We also do this operation AFTER depositing the reward event since it's possible some off-chain logic relies on
+            // event counting, and we want to avoid breaking it.
+            if refund_caller {
+                T::Currency::resolve_creating(&caller, refund_imbalance);
+            }
+
             Ok(Some(if should_restake_reward {
                 T::WeightInfo::claim_staker_with_restake()
             } else {

pallets/dapps-staking/src/tests.rs

@@ -2485,35 +2485,47 @@ fn claim_staker_for_works() {
         // Advance to next era so we can claim rewards for it
         advance_to_era(DappsStaking::current_era() + 1);
 
+        // 1. Call regular claim, and check the reward amount
         System::reset_events();
+        assert_ok!(DappsStaking::claim_staker(
+            RuntimeOrigin::signed(staker),
+            smart_contract.clone()
+        ));
+
+        let (event_staker, expected_reward) = match &System::events()[2].event {
+            mock::RuntimeEvent::DappsStaking(Event::Reward(staker, _, _, reward)) => (*staker, *reward),
+            _ => panic!("Unexpected event"),
+        };
+        assert_eq!(event_staker, staker);
+
+        // 2. Check that delegated claim works as expected
+        advance_to_era(DappsStaking::current_era() + 1);
+        System::reset_events();
+
         assert_ok!(DappsStaking::claim_staker_for(
             RuntimeOrigin::signed(claimer),
             staker,
             smart_contract.clone()
         ));
 
-        // We expect 5 events in total
+        // We expect 4 events in total
         // - 2 events related to reward withdraw & deposit
+        // - 1 events related to delegated claim fee deposit
         // - 1 event related to dApps staking reward
-        // - 2 events related to delegated claim fee withdraw & deposit
-        let events: Vec<_> = System::events();
-        assert_eq!(events.len(), 5);
+        let events = System::events();
+        assert_eq!(events.len(), 4);
 
+        // Deposit the reward to the caller. Reward must be reduced by the delegate claim fee amount.
+        let fee_amount = <TestRuntime as Config>::DelegateClaimFee::get();
+        assert!(expected_reward > fee_amount, "Reward must be greater than fee amount");
         assert_matches!(
             events[2].event,
-            mock::RuntimeEvent::DappsStaking(Event::Reward(staker, _, _, _)) if staker == staker
+            mock::RuntimeEvent::DappsStaking(Event::Reward(staker, _, _, reward)) if staker == staker && reward == (expected_reward - fee_amount)
         );
 
-        // Withdraw fee amount from the staker
-        let fee_amount = <TestRuntime as Config>::DelegateClaimFee::get();
+        // Deposit the call refund fee to the caller.
         assert_matches!(
             events[3].event,
-            mock::RuntimeEvent::Balances(pallet_balances::Event::Withdraw{who, amount}) if who == staker && amount == fee_amount
-        );
-
-        // ...and deposit it to the caller
-        assert_matches!(
-            events[4].event,
             mock::RuntimeEvent::Balances(pallet_balances::Event::Deposit{who, amount}) if who == claimer && amount == fee_amount
         );
     })