Testing 01 #1

WalquerX · 2023-03-21T21:19:03Z

Task One: Reward Beneficiary Delegation

Added set_reward_delegate and revoke_reward_delegate calls.
Modified claim_staker to handle delegation logic.
Added tests.

Dinonard

Good work!

I've left you some comments, and I'd like to hear your thoughts and opinion about them.

Dinonard · 2023-03-22T15:01:52Z

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

+    #[pallet::storage]
+    pub type RewardDelegate<T: Config> = StorageDoubleMap<
+        _,
+        Twox64Concat,


Why did you choose Twox64Concat and not e.g. Blake2_128Concat?

I was thinking about performance when I choose it. But giving it a second view since the user can kind of
arbitrarily choose the account ID that will be used as index It should be changed to Blake2_128Concat.

Dinonard · 2023-03-22T15:09:26Z

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

-            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));
+            if let Some(delegate) = RewardDelegate::<T>::get(&staker, &contract_id) {


Can this be solved in a more optimal way, in relationship to weight?

Since the delegation or not of the rewards are restricted by the restaking being set or not, I think a read to storage could be avoid in those cases.

Sure, that's true.

You also read in the value few lines above, so it would be more optimal to just reuse that.

Dinonard · 2023-03-22T15:12:42Z

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

+                // reward to the delegate
+                T::Currency::resolve_creating(&delegate, reward_imbalance);


What happens when restaking is used?

Since I changed should_restake_reward function to take into consideration if there is a delegate account, restaking will not be performed unless delegate account is not set, and in that case the reward will go to the staker.

Dinonard · 2023-03-22T15:20:21Z

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

+            // Ensure the staker does not have reward destination set to StakeBalance
+            let ledger = Self::ledger(&staker);
+            ensure!(
+                ledger.reward_destination != RewardDestination::StakeBalance,
+                Error::<T>::RewardDestinationStakeBalance
+            );


What if reward destination is changed later?

It will make no difference, because restaking and delegation now depends on delegate account being set or not. I though about creating a new reward destination so to make it more clear, but as it is implemented now, the rewards destinations are associated to the ledger, so are associated to an account. And for the delegation logic something smart contract specific was needed. I still think that is a possibility worth exploring.

Dinonard · 2023-03-22T15:24:10Z

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

        ) -> bool {
            reward_destination == RewardDestination::StakeBalance
                && dapp_state == DAppState::Registered
                && latest_staked_value > Zero::zero()
+                && delegate_account.is_none()


It seems a bit misleading to read from storage that staker has set reward destination to restake, but restake doesn't happen when claiming the reward.

What is your opinion on this?

Well, in the case when freebalance reward destination is set, the same happens. I mean, a read from storage is perfomed and no restaking is done. But I agree that the whole restaking logic could be done better, It doesn´t feel right that almost all claim_staker logic depends on "should restake" conditionals. But I would need to give it more time to think about a alternative implementation.

Dinonard · 2023-03-22T15:28:22Z

frame/dapps-staking/src/testing_utils.rs

+    // Check there is a delegate account
+    let delegate_account = RewardDelegate::<TestRuntime>::get(&claimer, &contract_id);
+


In case this is Some(account), would the function still work as expected?

In the case of Some(account) the rewards will go to freebalance destination. I´m missing a test to make sure this case will not create unexpected behaviour since current test doesn´t account for that.

Dinonard · 2023-03-22T15:29:23Z

frame/dapps-staking/src/testing_utils.rs

@@ -575,17 +579,156 @@ pub(crate) fn assert_claim_staker(claimer: AccountId, contract_id: &MockSmartCon
    );
 }

+pub(crate) fn assert_claim_staker_with_delegate(claimer: AccountId, delegated_account: AccountId, contract_id: &MockSmartContract<AccountId>) {


Good that you added such function!

Perhaps it would be better to keep it a single asset_claim_staker though?
Right now you've introduced the second public function for the same thing.

Yes, I should be simplified. I did it this way because I wanted to control the changes so to compare to the previous function. But It should be changed, also to avoid code duplication.

Dinonard · 2023-03-22T15:30:02Z

frame/dapps-staking/src/testing_utils.rs

+    let init_state_claim_era = MemorySnapshot::all(claim_era, contract_id, claimer);
+    let init_state_current_era = MemorySnapshot::all(current_era, contract_id, claimer);
+
+    let init_state_delegate_claim_era = MemorySnapshot::all(claim_era, contract_id, delegated_account);


Would there be benefit of adding delegate to the memory snapshot struct?

Yes, It would simplify the testing code. So not to make the delegating logic testing different from the other functions.

Dinonard · 2023-03-22T15:31:51Z

frame/dapps-staking/src/testing_utils.rs

+        init_state_claim_era.contract_info,
+        final_state_claim_era.contract_info
+    );
+}


That's a lot of duplicated code 🙂

Dinonard · 2023-03-22T15:35:13Z

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

+        /// Only staker or current delegate account can delegate reward
+        NotAuthorizedToDelegate,
+        /// Reward is already delegated to the account
+        AlreadyDelegated,
+        /// Can not set reward delegate if rewards are set to be re staked
+        RewardDestinationStakeBalance,
+        /// There is no reward delegate set
+        NoDelegateSet,


Have you tested these out?

No, I should have added a test for each event or error possible.

WalquerX added 4 commits March 17, 2023 01:45

main delegation logic

839b21c

adding pallet enabled conditional

d6a3b4c

adding test

37ef58b

adding comments

c51cdf2

Dinonard reviewed Mar 22, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Testing 01 #1

Testing 01 #1

WalquerX commented Mar 21, 2023

Dinonard left a comment

Dinonard Mar 22, 2023

WalquerX Mar 22, 2023

Dinonard Mar 22, 2023

WalquerX Mar 22, 2023

Dinonard Mar 23, 2023

Dinonard Mar 22, 2023

WalquerX Mar 22, 2023

Dinonard Mar 22, 2023

WalquerX Mar 22, 2023

Dinonard Mar 22, 2023

WalquerX Mar 22, 2023

Dinonard Mar 22, 2023

WalquerX Mar 22, 2023

Dinonard Mar 22, 2023

WalquerX Mar 22, 2023

Dinonard Mar 22, 2023

WalquerX Mar 22, 2023

Dinonard Mar 22, 2023

WalquerX Mar 22, 2023

Dinonard Mar 22, 2023

WalquerX Mar 22, 2023

		// reward to the delegate
		T::Currency::resolve_creating(&delegate, reward_imbalance);

		// Check there is a delegate account
		let delegate_account = RewardDelegate::<TestRuntime>::get(&claimer, &contract_id);

Testing 01 #1

Are you sure you want to change the base?

Testing 01 #1

Conversation

WalquerX commented Mar 21, 2023

Dinonard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment