Replace staking reserves with locks #1604

notlesh · 2022-06-14T21:01:44Z

What does it do?

Replaces parachain_staking's use of ReservableCurrency with LockableCurrency with the goal of allowing stakers to participate in governance.

Here is a good write-up on the difference between the two: https://stackoverflow.com/a/56418101

⚠️ Breaking Changes ⚠️

⚠️ parachain-staking now uses LockableCurrency instead of ReservableCurrency. Bonds will now use a lock with id stkngdel for delegators and stkngcol for collators rather than a reserve on their funds.
⚠️ Querying free_balance will behave differently for stakers. Previously this amount would exclude bonds, now it will include them. It is likely that this will reflect in cases where it did not before, such as Ethereum wallets like MetaMask.
⚠️ Users will still be unable to spend these bonded funds (transfer, pay for fees, etc.) while they are locked, but they will be able to do other operations which require a lock (e.g. they can vote with their staked funds).
⚠️ Migration Strategy: For existing bonds, there is a two-pronged migration strategy to replace reserves with locks.

One is explicit (see the new extrinsics hotfix_migrate_delegators_from_reserve_to_locks and hotfix_migrate_collators_from_reserve_to_locks), which can be used by either PureStake to migrate all bonds or by a user to migrate their own bond.
The other is JIT (just-in-time), which will migrate any account when a change to its bond occurs as needed.
It will not be necessary for any user, dapp, etc. to worry about migration except for a user who does not want to wait for bulk-migration and wants to migrate themselves.
We expect to migrate all accounts using these "hotfix extrinsics" immediately following a runtime upgrade, and this should be quick (10s to 100s of blocks).

TODO:

crystalin · 2022-06-14T21:07:54Z

I'm still not sure if we are going to use Lock/Unreserve for the self-bond. We might want to use reserve so it is "slashable" but I'm not sure what would be the condition (maybe for the rand key reveal slashing)

notlesh · 2022-06-14T21:08:45Z

I'm still not sure if we are going to use Lock/Unreserve for the self-bond. We might want to use reserve so it is "slashable" but I'm not sure what would be the condition (maybe for the rand key reveal slashing)

It'll be easy to remove that if we decide to do so.

notlesh · 2022-06-14T21:10:37Z

IMO, our governance should reflect the will of the collators, they are very important and act in the best interest of our chain.

crystalin · 2022-06-20T21:09:03Z

Things to verify:

Not possible to stake multiple times with the same token
Not possible to stake to different collators with same tokens (as opposed to democracy that allow to vote to multiple referendum with the same tokens)

I think those will require a separate storage to keep the count of those locked tokens

notlesh · 2022-06-20T22:04:53Z

I think those will require a separate storage to keep the count of those locked tokens

I'm assuming that a delegator's total locked amount should be their total bond amount, which we are already keeping synced at all times (this is the total across all of their delegations, so it shouldn't provide any "overlap" problems).

crystalin · 2022-07-04T02:04:59Z

Out of curiosity, why do we have 2 different locks for delegators and candidates ?

notlesh · 2022-07-04T17:23:27Z

Out of curiosity, why do we have 2 different locks for delegators and candidates ?

Good question. That started out as something between being defensive and unsure. I think we could get rid of it (combine the two) but I think it still has some small value for future-proofing. I don't have a strong opinion about it.

crystalin · 2022-07-04T17:45:43Z

I'm worried it might play against us if we remove by mistake the check for not being a candidate/delegator at the same time

pallets/parachain-staking/src/lib.rs

pallets/parachain-staking/src/types.rs

tests/contracts/compiled/CallPermit.json

Co-authored-by: girazoki <gorka.irazoki@gmail.com>

Co-authored-by: tgmichel <telmo@purestake.com>

notlesh · 2022-07-05T14:53:30Z

I'm worried it might play against us if we remove by mistake the check for not being a candidate/delegator at the same time

I've considered the same problem, but I think keeping them separate would help if something were to go wrong. What do you think would be improved by combining them?

girazoki

LGTM, I have the concern of additional DB reads/writes jit migration would add, did we take a decision on that already?

notlesh · 2022-07-05T18:13:05Z

LGTM, I have the concern of additional DB reads/writes jit migration would add, did we take a decision on that already?

Yes, I think everyone thinks this is acceptable as it is limited in several ways. Thanks for considering it though.

crystalin · 2022-07-05T18:18:41Z

pallets/author-mapping/src/migrations.rs

@@ -60,6 +60,8 @@ impl<T: Config> OnRuntimeUpgrade for AddAccountIdToNimbusLookup<T> {
 	#[cfg(feature = "try-runtime")]
 	fn pre_upgrade() -> Result<(), &'static str> {
 		use frame_support::traits::OnRuntimeUpgradeHelpersExt;
+		use sp_std::vec::Vec;


Is this needed ?

It was needed for try-runtime I believe, although that doesn't currently build (I think we need moonbeam-foundation/nimbus#62)

Replace (most) reserves with locks

c51db00

crystalin added A0-pleasereview Pull request needs code review. B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D9-needsaudit👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Jun 14, 2022

notlesh added A3-inprogress Pull request is in progress. No review needed at this stage. A0-pleasereview Pull request needs code review. and removed A0-pleasereview Pull request needs code review. labels Jun 14, 2022

notlesh added 3 commits June 20, 2022 17:13

fmt

fcc4be3

Merge branch 'master' into notlesh-staking-locks

72ee227

Convert last remaining reserve -> balance

7730daf

Crystalin and others added 15 commits June 20, 2022 20:06

Adds simple test for locks in staking (WIP)

e9b5338

Encapsulation of (most) delegator locking logic

ebe64c8

Convert collator reserve -> lock

9f8aa56

Remove SplitCandidateStateToDecreasePoV

b4b0b8b

fmt

3903575

editorconfig

e715709

wtf

dcc5e53

🚧

a91b021

Don't use free_balance with locks

a07d17f

Very basic migration

28f5a63

Hook up migration

343cb09

Fix try-runtime

bc752d1

Add logging to migration

bd0343f

Typo

04ce29d

Update tests to reflect reserve -> lock

e504851

Crystalin added 3 commits July 3, 2022 22:55

Adds more testing

6f1398b

Increase timeout for big staking tests

3a80a78

Improve timeouts

8b5e9bb

More timeout

7633690

tgmichel reviewed Jul 5, 2022

View reviewed changes

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

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

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

tests/contracts/compiled/CallPermit.json Show resolved Hide resolved

notlesh and others added 7 commits July 5, 2022 07:21

Update tests/tests/test-staking/test-staking-locks.ts

ad0d7fd

Co-authored-by: girazoki <gorka.irazoki@gmail.com>

Update pallets/parachain-staking/src/types.rs

afce2d3

Co-authored-by: girazoki <gorka.irazoki@gmail.com>

Update pallets/parachain-staking/src/types.rs

0227fda

Co-authored-by: girazoki <gorka.irazoki@gmail.com>

Use generic error instead of irrelevant one

d2baeda

Remove extra toString()

6c10b2c

Update pallets/parachain-staking/src/types.rs

53ad8a4

Co-authored-by: tgmichel <telmo@purestake.com>

Update pallets/parachain-staking/src/types.rs

5d6f934

Co-authored-by: tgmichel <telmo@purestake.com>

notlesh added 4 commits July 5, 2022 14:56

Use LockIdentifier type alias

004ceaf

Use usable_balance to include locks in test

ded490e

fmt

c98547f

Revent default_with_total

10abaee

girazoki approved these changes Jul 5, 2022

View reviewed changes

crystalin mentioned this pull request Jul 5, 2022

runtime-1700 release #1659

Closed

14 tasks

crystalin approved these changes Jul 5, 2022

View reviewed changes

notlesh merged commit 8ec7a31 into master Jul 5, 2022

notlesh deleted the notlesh-staking-locks branch July 5, 2022 18:23

lsaether mentioned this pull request Jul 19, 2022

What is required to make Moonbeam's staking pallet allow locked tokens to be staked? zeitgeistpm/zeitgeist#715

Closed

notlesh added D1-audited👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D9-needsaudit👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Jul 27, 2022

nvengal mentioned this pull request Jul 28, 2022

Update from upstream/master OAK-Foundation/moonbeam#15

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace staking reserves with locks #1604

Replace staking reserves with locks #1604

notlesh commented Jun 14, 2022 •

edited

crystalin commented Jun 14, 2022

notlesh commented Jun 14, 2022

notlesh commented Jun 14, 2022

crystalin commented Jun 20, 2022

notlesh commented Jun 20, 2022

crystalin commented Jul 4, 2022

notlesh commented Jul 4, 2022

crystalin commented Jul 4, 2022

notlesh commented Jul 5, 2022

girazoki left a comment

notlesh commented Jul 5, 2022

crystalin Jul 5, 2022

notlesh Jul 5, 2022

Replace staking reserves with locks #1604

Replace staking reserves with locks #1604

Conversation

notlesh commented Jun 14, 2022 • edited

What does it do?

⚠️ Breaking Changes ⚠️

TODO:

crystalin commented Jun 14, 2022

notlesh commented Jun 14, 2022

notlesh commented Jun 14, 2022

crystalin commented Jun 20, 2022

notlesh commented Jun 20, 2022

crystalin commented Jul 4, 2022

notlesh commented Jul 4, 2022

crystalin commented Jul 4, 2022

notlesh commented Jul 5, 2022

girazoki left a comment

Choose a reason for hiding this comment

notlesh commented Jul 5, 2022

crystalin Jul 5, 2022

Choose a reason for hiding this comment

notlesh Jul 5, 2022

Choose a reason for hiding this comment

notlesh commented Jun 14, 2022 •

edited