[Pending] Rebond unlocking chunks #97

shunsukew · 2022-09-15T06:32:29Z

Solves #63

Pull Request Summary
Added feature rebond_and_stake unlocking chunks to contracts.

Once stakers start the unbonding, their unbonded funds will remain locked during the unbonding period. During this period, these funds neither generate any rewards nor can they be used for payment, transfer, etc.

One functionality we could provide is for stakers to re-bond their unbonding chunks. They could select unbonding chunks as a source when staking on a contract from UI, similar as they do for nomination_transfer. This will result in unbonding chunks being consumed and (re)staked on some contract.

Check list

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

shunsukew · 2022-09-19T10:39:16Z

compilation started to fail after pushing small commit 852fb39. Why, will check

chain-extensions/dapps-staking/src/lib.rs

chain-extensions/types/dapps-staking/src/lib.rs

0x7CFE · 2022-09-19T10:12:36Z

frame/dapps-staking/src/benchmarking.rs

+    rebond_and_stake {
+        initialize::<T>();
+
+        let (_, contract_id) = register_contract::<T>(1)?;
+        prepare_bond_and_stake::<T>(T::MaxNumberOfStakersPerContract::get() - 1, &contract_id, SEED)?;
+
+        let staker = whitelisted_caller();
+        let _ = T::Currency::make_free_balance_be(&staker, BalanceOf::<T>::max_value());
+        let stake_amount = BalanceOf::<T>::max_value() / 2u32.into();
+        let unstake_amount = stake_amount / 2u32.into();
+
+        DappsStaking::<T>::bond_and_stake(RawOrigin::Signed(staker.clone()).into(), contract_id.clone(), stake_amount)?;
+        DappsStaking::<T>::unbond_and_unstake(RawOrigin::Signed(staker.clone()).into(), contract_id.clone(), unstake_amount)?;
+    }: _(RawOrigin::Signed(staker.clone()), contract_id.clone())
+    verify {
+        assert_last_event::<T>(Event::<T>::RebondAndStake(staker, contract_id, unstake_amount).into());
+    }


nit: I'd like to have some comments describing what is actually happening.

0x7CFE

LGTM aside of small nits

0x7CFE · 2022-09-22T09:01:49Z

chain-extensions/dapps-staking/src/lib.rs

+                let base_weight =
+                    <T as pallet_dapps_staking::Config>::WeightInfo::rebond_and_stake();
+                env.charge_weight(base_weight)?;
+
+                let caller = env.ext().address().clone();
+                let call_result = pallet_dapps_staking::Pallet::<T>::rebond_and_stake(
+                    RawOrigin::Signed(caller).into(),
+                    contract,
+                );


Was kinda confused since rebond_and_stake is called twice in a row. I do understand that these are two different calls, but still this is a bit confusing. Maybe we should add couple of comments to reduce overall WTF/minute.

This can be said for other functions too. it's a more universal topic.
I thought it's very clear they are different and what they do because they are different methods, one is WeightInfo's and another is Pallet's.

0x7CFE · 2022-09-22T09:03:07Z

chain-extensions/types/dapps-staking/src/lib.rs

@@ -120,27 +123,29 @@ pub enum Contract<Account> {
    Wasm(Account),
 }

+pub type ContractBytes = [u8; 32];


Since this type is in public it would be a good idea to add some explanatory comment. What is this type, why it 32 bytes long, what is the purpose of that type, etc.

github-actions · 2022-09-23T04:33:33Z

Package	Line Rate	Branch Rate	Health
chain-extensions/trait/src	0%	0%	❌
chain-extensions/rmrk/src	0%	0%	❌
chain-extensions/types/dapps-staking/src	0%	0%	❌
contracts/xvm/src	0%	0%	❌
frame/dapps-staking/src/pallet	90%	0%	✔
frame/block-reward/src	96%	0%	✔
frame/pallet-xcm/src	81%	0%	✔
chain-extensions/xvm/src	0%	0%	❌
frame/pallet-xvm/src	9%	0%	❌
precompiles/utils/macro/src	0%	0%	❌
precompiles/utils/src/data	72%	0%	➖
chain-extensions/dapps-staking/src	0%	0%	❌
frame/collator-selection/src	88%	0%	✔
precompiles/substrate-ecdsa/src	78%	0%	➖
precompiles/xvm/src	71%	0%	➖
primitives/xcm/src	88%	0%	✔
precompiles/utils/src	84%	0%	✔
chain-extensions/types/rmrk/src	0%	0%	❌
frame/pallet-xvm/src/pallet	37%	0%	❌
precompiles/assets-erc20/src	91%	0%	✔
frame/dapps-staking/src	94%	0%	✔
precompiles/dapps-staking/src	95%	0%	✔
precompiles/sr25519/src	78%	0%	➖
chain-extensions/types/xvm/src	0%	0%	❌
precompiles/utils/macro/tests	100%	0%	✔
precompiles/xcm/src	80%	0%	➖
frame/xc-asset-config/src	87%	0%	✔
frame/custom-signatures/src	81%	0%	✔
Summary	80% (7588 / 9480)	0% (0 / 0)	✔

Minimum allowed line rate is 60%

shunsukew · 2022-09-28T04:38:54Z

@Dinonard I also added chain extension and precompiles, but I started to think they should be separately released (in a different PR) after this function become stable. What do you think?

Dinonard

Thanks for working on this!

About your precompile/chain-extension questions, I think we should keep them out of this PR. And maybe we shouldn't add them at all.
Your comment about feature being stable makes sense and I'd also add the point that we'll be redesigning dapps-staking very soon. It's not necessarily true that this function will be compatible with new version so I'd wait until then with it. That way we reduce number of incompatible legacy functions.

Dinonard · 2022-10-17T06:25:41Z

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

+        ///
+        /// All unbonding chunks will be used and staked to the specified contract.
+        ///
+        /// The dispatch origin for this call must be _Signed_ by the staker's account.


Please also check top of lib.rs file and update comments there if needed.

Dinonard · 2022-10-17T06:30:34Z

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

+            GeneralEraInfo::<T>::mutate(&current_era, |value| {
+                if let Some(x) = value {
+                    x.staked = x.staked.saturating_add(value_to_stake);
+                    x.locked = x.locked.saturating_add(value_to_stake);


This is incorrect - TVL doesn't increase after this since unbonding chunks are still considered to be locked.

UT should be updated to catch this.

Dinonard · 2022-10-17T06:33:40Z

frame/dapps-staking/src/testing_utils.rs

+    assert_eq!(
+        final_state.era_info.locked,
+        init_state.era_info.locked + expected_stake_amount
+    );


This is incorrect, see my comment above.

Dinonard · 2022-10-17T06:35:09Z

frame/dapps-staking/src/tests.rs

+        );
+    })
+}
+


I'd suggest also covering the error cases with InsufficientValue or TooManyEraStakeValues.

Not mandatory, just a suggestion.

shunsukew · 2022-10-19T05:00:47Z

Thanks for working on this!

About your precompile/chain-extension questions, I think we should keep them out of this PR. And maybe we shouldn't add them at all. Your comment about feature being stable makes sense and I'd also add the point that we'll be redesigning dapps-staking very soon. It's not necessarily true that this function will be compatible with new version so I'd wait until then with it. That way we reduce number of incompatible legacy functions.

That makes sense.
In that case, I'd make this PR pending until the redesign and its implementation finish. I'll resume this PR work after that.

Dinonard · 2022-10-19T06:26:08Z

Thanks for working on this!
About your precompile/chain-extension questions, I think we should keep them out of this PR. And maybe we shouldn't add them at all. Your comment about feature being stable makes sense and I'd also add the point that we'll be redesigning dapps-staking very soon. It's not necessarily true that this function will be compatible with new version so I'd wait until then with it. That way we reduce number of incompatible legacy functions.

That makes sense. In that case, I'd make this PR pending until the redesign and its implementation finish. I'll resume this PR work after that.

I'm not sure how feasible is that, tbh. We don't know if this will even be applicable then.

I don't see an issue with this being merged now but we should avoid precompiles/chain-extensions. Also, not sure how much it makes sense to ask UI team to do this since it will most likely be changed soon.

Anyhow, draft status that you used is probably the way to go.

rebond unlocking chunks

45b53b3

shunsukew changed the title ~~rebond unlocking chunks~~ [WIP] Rebond unlocking chunks Sep 15, 2022

Dinonard suggested changes Sep 15, 2022

View reviewed changes

frame/dapps-staking/src/pallet/mod.rs Outdated Show resolved Hide resolved

shunsukew added 10 commits September 15, 2022 19:51

up

04a6b1f

up

de3ff1e

cargo fmt

f36013d

added precompiles and chain extensions

86a2c09

cargo fmt

c391f65

benchmarking

f27a1ad

fix bench

292cfee

fix bench

719024d

updated weights info

93286fc

fix locked amount

7059ffa

shunsukew requested a review from Dinonard September 16, 2022 07:15

shunsukew changed the title ~~[WIP] Rebond unlocking chunks~~ Rebond unlocking chunks Sep 16, 2022

shunsukew marked this pull request as ready for review September 16, 2022 07:15

shunsukew requested a review from akru September 16, 2022 12:14

update test

852fb39

hoonsubin requested a review from 0x7CFE September 19, 2022 08:25

0x7CFE reviewed Sep 19, 2022

View reviewed changes

shunsukew added 3 commits September 21, 2022 15:37

Merge branch 'polkadot-v0.9.28' into feature/ds-rebond-unlocking-chunks

0470a5e

up

7a8f3b5

cargo fmt

5c36af9

shunsukew requested a review from 0x7CFE September 21, 2022 14:15

0x7CFE reviewed Sep 22, 2022

View reviewed changes

shunsukew changed the base branch from polkadot-v0.9.28 to polkadot-v0.9.29 September 23, 2022 04:15

shunsukew added 2 commits September 23, 2022 13:16

Merge branch 'polkadot-v0.9.29' into feature/ds-rebond-unlocking-chunks

89cf22b

changed branch base to polkadot-v0.9.29

a3d98b1

Dinonard reviewed Oct 17, 2022

View reviewed changes

shunsukew changed the title ~~Rebond unlocking chunks~~ [Pending] Rebond unlocking chunks Oct 19, 2022

shunsukew marked this pull request as draft October 19, 2022 05:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Pending] Rebond unlocking chunks #97

[Pending] Rebond unlocking chunks #97

shunsukew commented Sep 15, 2022 •

edited

shunsukew commented Sep 19, 2022

0x7CFE Sep 19, 2022

0x7CFE left a comment

0x7CFE Sep 22, 2022

shunsukew Sep 28, 2022 •

edited

0x7CFE Sep 22, 2022

github-actions bot commented Sep 23, 2022

shunsukew commented Sep 28, 2022 •

edited

Dinonard left a comment

Dinonard Oct 17, 2022

Dinonard Oct 17, 2022

Dinonard Oct 17, 2022

Dinonard Oct 17, 2022

shunsukew commented Oct 19, 2022 •

edited

Dinonard commented Oct 19, 2022

[Pending] Rebond unlocking chunks #97

Are you sure you want to change the base?

[Pending] Rebond unlocking chunks #97

Conversation

shunsukew commented Sep 15, 2022 • edited

shunsukew commented Sep 19, 2022

0x7CFE Sep 19, 2022

Choose a reason for hiding this comment

0x7CFE left a comment

Choose a reason for hiding this comment

0x7CFE Sep 22, 2022

Choose a reason for hiding this comment

shunsukew Sep 28, 2022 • edited

Choose a reason for hiding this comment

0x7CFE Sep 22, 2022

Choose a reason for hiding this comment

github-actions bot commented Sep 23, 2022

shunsukew commented Sep 28, 2022 • edited

Dinonard left a comment

Choose a reason for hiding this comment

Dinonard Oct 17, 2022

Choose a reason for hiding this comment

Dinonard Oct 17, 2022

Choose a reason for hiding this comment

Dinonard Oct 17, 2022

Choose a reason for hiding this comment

Dinonard Oct 17, 2022

Choose a reason for hiding this comment

shunsukew commented Oct 19, 2022 • edited

Dinonard commented Oct 19, 2022

shunsukew commented Sep 15, 2022 •

edited

shunsukew Sep 28, 2022 •

edited

shunsukew commented Sep 28, 2022 •

edited

shunsukew commented Oct 19, 2022 •

edited