feat: remove multiple delegations #391

wischli · 2022-08-01T16:13:01Z

No Ticket - part of #384

This PR removes unused code from the staking pallet, mainly the unusable delegate_another_candidate extrinsic, revoke_delegation extrinsic and two useless staking parameters.

While working on #384 I just stumbled the problem in the current redesign introduced by allowing for more than one delegation per delegator (which we do in the unit test environment). As discussed, we don't seem to plan supporting multiple delegations from the same account in the future.

What was removed?

delegate_another_candidate extrinsic
revoke_delegation extrinsic (not needed because of leave_delegators)
Comments about the weight of functions in the Staking Pallet
Delegator struct (it's just Stake)
ReplacedDelegator struct, not needed anymore
NomStakeBelowMin error (redundant because of DelegationBelowMin)
prep_kick_delegator function --> was only required because of potentially still existent delegations of kicked delegator

Open tasks

The benchmark unit tests are currently failing for the staking pallet --> feat: staking payout refactor #384.
Writing a migration for the breaking DelegatorState Storage Map --> feat: staking payout refactor #384.

Checklist:

I have verified that the code works
- No panics! (checked arithmetic ops, no indexing array[3] use get(3), ...)
I have verified that the code is easy to understand
- If not, I have left a well-balanced amount of inline comments
I have left the code in a better state
I have documented the changes (where applicable)

wischli

Some comments which should simplify your review.

wischli · 2022-08-01T16:22:07Z

pallets/parachain-staking/src/lib.rs


-		/// Minimum stake required for any account to be able to delegate.
-		#[pallet::constant]
-		type MinDelegation: Get<BalanceOf<Self>>;


This parameter was always redundant because of MinDelegatorStake.

wischli · 2022-08-01T16:22:27Z

pallets/parachain-staking/src/lib.rs

-		OptionQuery,
-	>;
+	pub(crate) type DelegatorState<T: Config> =
+		StorageMap<_, Twox64Concat, T::AccountId, Delegator<T::AccountId, BalanceOf<T>>, OptionQuery>;


Migration will be part of #384

wischli · 2022-08-01T16:22:56Z

pallets/parachain-staking/src/lib.rs

 		///
 		/// The dispatch origin must be Root.
-		///
-		/// # <weight>


Removed all of these because they are super outdated.

wischli · 2022-08-01T16:23:58Z

pallets/parachain-staking/src/lib.rs

-			let delegator_state = Delegator::try_new(collator.clone(), amount)
-				.map_err(|_| Error::<T>::MaxCollatorsPerDelegatorExceeded)?;
-
+			let delegator_state = Delegator {


We had to try before, because a DelegatorState included an ordered set of delegations. Now it's just { amount: BalanceOf<T>, owner: Option<AccountId> }.

wischli · 2022-08-01T16:24:47Z

pallets/parachain-staking/src/lib.rs

 				Self::do_update_delegator(delegation, state)?
 			} else {
 				state.total = state.total.saturating_add(amount);
-				(state, None)


We don't need to care about the kicked delegator anymore because they cannot have other delegations anymore.

wischli · 2022-08-01T16:26:14Z

pallets/parachain-staking/src/lib.rs

-		/// - Kills: DelegatorState if the delegator has not delegated to
-		///   another collator
-		/// # </weight>
-		fn delegator_revokes_collator(acc: T::AccountId, collator: T::AccountId) -> Result<u32, DispatchError> {


Not required anymore because of delegator_leaves_collator.

wischli · 2022-08-01T16:26:35Z

pallets/parachain-staking/src/lib.rs

-		///   remaining
-		/// * Or `None`, signalling the delegator state should be cleared once
-		///   the transaction cannot fail anymore.
-		fn prep_kick_delegator(


Not required anymore because the replaced delegator cannot have further delegations.

wischli · 2022-08-01T16:26:57Z

pallets/parachain-staking/src/lib.rs

-
-		/// Either clear the storage of a kicked delegator or update its
-		/// delegation state if it still contains other delegations.
-		fn update_kicked_delegator_storage(delegator: Option<ReplacedDelegator<T>>) {


When kicking a delegator, we kill their DelegatorState storage.

wischli · 2022-08-01T16:29:06Z

pallets/parachain-staking/src/types.rs

-}
-
-impl<AccountId, Balance, MaxCollatorsPerDelegator> Delegator<AccountId, Balance, MaxCollatorsPerDelegator>
+pub type Delegator<AccountId, Balance> = Stake<Option<AccountId>, Balance>;


We need to wrap the collator accountid into an option to differentiate between default (empty) delegator states. Unfortunately (or luckily!), AccountId does and should not implement Default anymore.

ntn-x2

Didn't check the unit tests, but otherwise the changes make sense to me.

pallets/parachain-staking/src/lib.rs

Co-authored-by: Antonio <[email protected]>

wischli · 2022-08-03T16:08:04Z

On a note: I am done with #384 (based on the state of this branch) but havent pushed it to the branch yet (instead wf/1930-staking-refactor-tmp) until @weichweich found the time to review here. Take all the time you need as this is a low hanging fruit. Or, if you prefer, I can also push everything to #384 and you review everything at once.

weichweich

LGTM

@ntn-x2

* wip: poc * refactor: extrinsics and inherents * refactor: remove storage redundancy * tests: fix existing ones for staking * tests: 80% * feat: rm delegate_another_candidate and more * docs: remove deprecated weight * Apply suggestions from code review Co-authored-by: Antonio <[email protected]> * tests: 100% * fix: benchmarks * feat: remove multiple delegations (#391) * tests: 80% * feat: rm delegate_another_candidate and more * docs: remove deprecated weight * Apply suggestions from code review Co-authored-by: Antonio <[email protected]> Co-authored-by: Antonio <[email protected]> * Apply suggestions from code review Co-authored-by: Antonio <[email protected]> * fix: suggestions from @ntn-x2 review * fmt * fix: typos * fix: docker file * feat: add staking rewards runtime api * docs: add new extrinsics to staking header * feat: add staking rates api * tests: add api * fix: unify staking runtime api * refactor: stricter claiming * fix: easy suggestions from code review * tests: comp api claim with claiming * stlye: fmt * tests: add safety check to API rewards * style: fmt * feat: write delegator state migration * refactor: Delegator interface functions * refactor: rm Option for Delegator.owner * style: apply turbofish to staking * fix: replace RewardCount with two counters * refactor: rm col input for del stake adjustment * Update pallets/parachain-staking/src/lib.rs Co-authored-by: Albrecht <[email protected]> Co-authored-by: Antonio <[email protected]> Co-authored-by: Albrecht <[email protected]>

@ntn-x2

* wip: poc * refactor: extrinsics and inherents * refactor: remove storage redundancy * tests: fix existing ones for staking * tests: 80% * feat: rm delegate_another_candidate and more * docs: remove deprecated weight * Apply suggestions from code review Co-authored-by: Antonio <[email protected]> * tests: 100% * fix: benchmarks * feat: remove multiple delegations (#391) * tests: 80% * feat: rm delegate_another_candidate and more * docs: remove deprecated weight * Apply suggestions from code review Co-authored-by: Antonio <[email protected]> Co-authored-by: Antonio <[email protected]> * Apply suggestions from code review Co-authored-by: Antonio <[email protected]> * fix: suggestions from @ntn-x2 review * fmt * fix: typos * fix: docker file * feat: add staking rewards runtime api * docs: add new extrinsics to staking header * feat: add staking rates api * tests: add api * fix: unify staking runtime api * refactor: stricter claiming * fix: easy suggestions from code review * refactor: separate runtime api from custom rpc * tests: comp api claim with claiming * refactor: move runtime api calls to mod * stlye: fmt * refactor: split api into separate crates * refactor: simplify staking api name * refactor: normalize public creds runtime api * refactor: machete cleanup * tests: add safety check to API rewards * style: fmt * feat: write delegator state migration * refactor: remove unneeded no_std flags * refactor: Delegator interface functions * refactor: rm Option for Delegator.owner * style: apply turbofish to staking * fix: replace RewardCount with two counters * refactor: rm col input for del stake adjustment * style: fmt Co-authored-by: Antonio <[email protected]>

wischli added 3 commits August 1, 2022 11:15

tests: 80%

791ef9c

feat: rm delegate_another_candidate and more

38e92dd

docs: remove deprecated weight

d8bb955

wischli requested review from ntn-x2 and weichweich August 1, 2022 16:21

wischli commented Aug 1, 2022

View reviewed changes

ntn-x2 reviewed Aug 2, 2022

View reviewed changes

pallets/parachain-staking/src/lib.rs Outdated Show resolved Hide resolved

pallets/parachain-staking/src/lib.rs Outdated Show resolved Hide resolved

Apply suggestions from code review

24dc4bf

Co-authored-by: Antonio <[email protected]>

weichweich approved these changes Aug 8, 2022

View reviewed changes

wischli merged commit 8926e4c into wf/1930-staking-refactor Aug 8, 2022

wischli deleted the wf/rm-multiple-delegations branch August 8, 2022 08:13

feat: remove multiple delegations #391

feat: remove multiple delegations #391

Uh oh!

Conversation

wischli commented Aug 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

No Ticket - part of #384

What was removed?

Open tasks

Checklist:

Uh oh!

wischli left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ntn-x2 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

wischli commented Aug 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weichweich left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wischli commented Aug 1, 2022 •

edited

Loading

wischli commented Aug 3, 2022 •

edited

Loading