-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Incentive vesting for validators #12210
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
3eb6686
fbf6b23
37194eb
0e42a92
a9b20ed
203bca9
615e6fa
0249fb1
f43223a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -231,6 +231,7 @@ impl frame_system::Config for Runtime { | |
| type SingleBlockMigrations = Migrations; | ||
| type OnNewAccount = pallet_revive::AutoMapper<Runtime>; | ||
| type OnKilledAccount = pallet_revive::AutoMapper<Runtime>; | ||
| type BaseCallFilter = ValidatorVestingCallFilter; | ||
| } | ||
|
|
||
| impl cumulus_pallet_weight_reclaim::Config for Runtime { | ||
|
|
@@ -540,6 +541,8 @@ parameter_types! { | |
| pub const MinVestedTransfer: Balance = 100 * CENTS; | ||
| pub UnvestedFundsAllowedWithdrawReasons: WithdrawReasons = | ||
| WithdrawReasons::except(WithdrawReasons::TRANSFER | WithdrawReasons::RESERVE); | ||
| pub const VestingLockId: frame_support::traits::LockIdentifier = | ||
| pallet_vesting::DEFAULT_VESTING_LOCK_ID; | ||
| } | ||
|
|
||
| impl pallet_vesting::Config for Runtime { | ||
|
|
@@ -551,6 +554,33 @@ impl pallet_vesting::Config for Runtime { | |
| type RuntimeEvent = RuntimeEvent; | ||
| type WeightInfo = weights::pallet_vesting::WeightInfo<Runtime>; | ||
| type UnvestedFundsAllowedWithdrawReasons = UnvestedFundsAllowedWithdrawReasons; | ||
| type LockId = VestingLockId; | ||
| } | ||
|
|
||
| parameter_types! { | ||
| pub const ValidatorVestingLockId: frame_support::traits::LockIdentifier = *b"stkinctv"; | ||
| } | ||
|
|
||
| /// Blocks direct user access to `vested_transfer` on the validator-incentive vesting instance. We | ||
| /// don't include `force_vested_transfer` because it requires root origin, which bypasses all | ||
| /// filters. | ||
| pub struct ValidatorVestingCallFilter; | ||
| impl frame_support::traits::Contains<RuntimeCall> for ValidatorVestingCallFilter { | ||
| fn contains(call: &RuntimeCall) -> bool { | ||
| !matches!(call, RuntimeCall::ValidatorVesting(pallet_vesting::Call::vested_transfer { .. })) | ||
| } | ||
| } | ||
|
|
||
| impl pallet_vesting::Config<pallet_vesting::Instance1> for Runtime { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know this was suggested elsewhere, but I'm wondering if adding a separate vesting instance is overkill, given:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. as we discussed - I think it's worth exploring
This way we also don't break UI - which is nice. One thing to take into account (see #11876 (comment) but there we are not taking validators-only into account) is to check how many validators are close to Again, this would be an exploration task. Might very well be that after that, we realize that the isolation / multiple instance approach is a lower-risk / cleaner approach (and if we go for that, then - as mentioned in another comment - let's go for a system/protocol instance of the vesting pallet and not a staking-incentive-specific one) |
||
| const MAX_VESTING_SCHEDULES: u32 = 100; | ||
| type BlockNumberProvider = RelaychainDataProvider<Runtime>; | ||
| type BlockNumberToBalance = ConvertInto; | ||
| type Currency = Balances; | ||
| type MinVestedTransfer = MinVestedTransfer; | ||
| type RuntimeEvent = RuntimeEvent; | ||
| type WeightInfo = weights::pallet_vesting::WeightInfo<Runtime>; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we update this one to point |
||
| type UnvestedFundsAllowedWithdrawReasons = UnvestedFundsAllowedWithdrawReasons; | ||
| type LockId = ValidatorVestingLockId; | ||
| } | ||
|
|
||
| parameter_types! { | ||
|
|
@@ -1766,6 +1796,7 @@ construct_runtime!( | |
| AssetTxPayment: pallet_asset_conversion_tx_payment = 13, | ||
| Vesting: pallet_vesting = 14, | ||
| PgasAllowance: pallet_pgas_allowance = 15, | ||
| ValidatorVesting: pallet_vesting::<Instance1> = 16, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Even if we go the route of a new vesting pallet, maybe it should be for other system-related vesting in general, not specifically for validators only. (Not saying anything is wrong, but we should really reason through why we're doing this and understand the tradeoffs before adding a new pallet.) |
||
|
|
||
| // Collator support. the order of these 5 are important and shall not change. | ||
| Authorship: pallet_authorship = 20, | ||
|
|
@@ -2259,6 +2290,7 @@ mod benches { | |
| [cumulus_pallet_xcmp_queue, XcmpQueue] | ||
| [pallet_treasury, Treasury] | ||
| [pallet_vesting, Vesting] | ||
| [pallet_vesting, ValidatorVesting] | ||
| [pallet_vesting_precompiles, VestingPrecompiles] | ||
| [pallet_whitelist, Whitelist] | ||
| [pallet_xcm_bridge_hub_router, ToRococo] | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -274,6 +274,7 @@ parameter_types! { | |
| pub const MaxNominations: u32 = <NposCompactSolution16 as frame_election_provider_support::NposSolution>::LIMIT as u32; | ||
| pub const MaxEraDuration: u64 = RelaySessionDuration::get() as u64 * RELAY_CHAIN_SLOT_DURATION_MILLIS as u64 * SessionsPerEra::get() as u64; | ||
| pub MaxPruningItems: u32 = 100; | ||
| pub const ValidatorIncentiveVestingDuration: BlockNumber = 365 * RC_DAYS; | ||
| } | ||
|
|
||
| impl pallet_staking_async::Config for Runtime { | ||
|
|
@@ -312,6 +313,12 @@ impl pallet_staking_async::Config for Runtime { | |
| pallet_staking_async::reward::DefaultStakerRewardCalculator<Runtime>; | ||
| type MaxPruningItems = MaxPruningItems; | ||
| type WeightInfo = weights::pallet_staking_async::WeightInfo<Runtime>; | ||
| type VestingDuration = ValidatorIncentiveVestingDuration; | ||
| type VestingBlockNumberProvider = RelaychainDataProvider<Runtime>; | ||
| type ValidatorIncentivePayout = pallet_staking_async::VestedIncentivePayout< | ||
| Balances, | ||
| pallet_vesting::Pallet<Runtime, pallet_vesting::Instance1>, | ||
| >; | ||
| } | ||
|
|
||
| // Relay Chain session keys type for validating session keys on AssetHub. | ||
|
|
@@ -645,6 +652,7 @@ where | |
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
| use frame_support::traits::Contains; | ||
|
|
||
| #[test] | ||
| fn all_epmb_weights_sane() { | ||
|
|
@@ -657,4 +665,87 @@ mod tests { | |
| ); | ||
| }) | ||
| } | ||
|
|
||
| #[test] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do you think it would be worth adding for each runtime configuring vesting pallet (or at least on WAH and similarly on Polkadot AH when time comes) a test checking that Today we are comfortably satisfying the constraint but since we are talking of configurations coming from different pallets, if someone change one of the knobs (VestingDuration, BondingDuration etc), we would end up in vesting almost silently degraded (validators start hitting AtMaxVestingSchedules and get liquid payouts only a per-payout |
||
| fn validator_vesting_call_filter_blocks_vested_transfer() { | ||
| // Permisionless `vested_transfer` is not allowed on the ValidatorVesting instance. | ||
| let call = RuntimeCall::ValidatorVesting(pallet_vesting::Call::vested_transfer { | ||
| target: AccountId::from([42u8; 32]).into(), | ||
| schedule: pallet_vesting::VestingInfo::new(MinVestedTransfer::get(), 1, 0), | ||
| }); | ||
| assert!(!ValidatorVestingCallFilter::contains(&call)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn validator_vesting_call_filter_allows_force_vested_transfer() { | ||
| // Since `force_vested_transfer` is root-only, it should always be accessible since | ||
| // it bypasses all filters. | ||
| let call = RuntimeCall::ValidatorVesting(pallet_vesting::Call::force_vested_transfer { | ||
| source: AccountId::from([1u8; 32]).into(), | ||
| target: AccountId::from([2u8; 32]).into(), | ||
| schedule: pallet_vesting::VestingInfo::new(MinVestedTransfer::get(), 1, 0), | ||
| }); | ||
| assert!(ValidatorVestingCallFilter::contains(&call)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn validator_vesting_call_filter_allows_user_facing_calls() { | ||
| // We must keep `vest` and `vest_other` open so holders can unlock their funds. | ||
| assert!(ValidatorVestingCallFilter::contains(&RuntimeCall::ValidatorVesting( | ||
| pallet_vesting::Call::vest {} | ||
| ))); | ||
| assert!(ValidatorVestingCallFilter::contains(&RuntimeCall::ValidatorVesting( | ||
| pallet_vesting::Call::vest_other { target: AccountId::from([1u8; 32]).into() } | ||
| ))); | ||
| assert!(ValidatorVestingCallFilter::contains(&RuntimeCall::ValidatorVesting( | ||
| pallet_vesting::Call::merge_schedules { schedule1_index: 0, schedule2_index: 1 } | ||
| ))); | ||
| // An unrelated pallet must also pass through. | ||
| assert!(ValidatorVestingCallFilter::contains(&RuntimeCall::Timestamp( | ||
| pallet_timestamp::Call::set { now: 0 } | ||
| ))); | ||
| } | ||
|
|
||
| #[test] | ||
| fn add_to_vesting_works_bypassing_call_filter() { | ||
| // Since `add_to_vesting` is a plain internal Rust call (and not a dispatchable) it is | ||
| // always allowed as it does not go through filtering. | ||
| use frame_support::traits::tokens::VestedPayout; | ||
| sp_io::TestExternalities::default().execute_with(|| { | ||
| let source = AccountId::from([1u8; 32]); | ||
| let dest = AccountId::from([2u8; 32]); | ||
| let amount = MinVestedTransfer::get(); | ||
|
|
||
| frame_support::assert_ok!(Balances::force_set_balance( | ||
| RuntimeOrigin::root(), | ||
| source.clone().into(), | ||
| amount + ExistentialDeposit::get(), | ||
| )); | ||
|
|
||
| frame_support::assert_ok!( | ||
| <pallet_vesting::Pallet<Runtime, pallet_vesting::Instance1> as VestedPayout< | ||
| AccountId, | ||
| Balance, | ||
| >>::add_to_vesting(&source, &dest, amount, 20u32, 1u32,) | ||
| ); | ||
|
|
||
| assert!( | ||
| pallet_vesting::Vesting::<Runtime, pallet_vesting::Instance1>::get(&dest).is_some() | ||
| ); | ||
| }); | ||
| } | ||
|
|
||
| #[test] | ||
| fn validator_vesting_call_filter_is_the_base_call_filter() { | ||
| // Verify that the runtime's BaseCallFilter is our filter, not `Everything`. This | ||
| // ensures the dispatchable-blocking is actually wired into the extrinsic pipeline. | ||
| let blocked = RuntimeCall::ValidatorVesting(pallet_vesting::Call::vested_transfer { | ||
| target: AccountId::from([0u8; 32]).into(), | ||
| schedule: pallet_vesting::VestingInfo::new(MinVestedTransfer::get(), 1, 0), | ||
| }); | ||
| assert!( | ||
| !<Runtime as frame_system::Config>::BaseCallFilter::contains(&blocked), | ||
| "BaseCallFilter must block ValidatorVesting::vested_transfer" | ||
| ); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -262,4 +262,12 @@ impl<T: frame_system::Config> pallet_vesting::WeightInfo for WeightInfo<T> { | |
| .saturating_add(T::DbWeight::get().reads(4_u64)) | ||
| .saturating_add(T::DbWeight::get().writes(3_u64)) | ||
| } | ||
| /// NOTE: placeholder — re-run benchmarks after merging. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess run before merging :D |
||
| fn add_to_vesting_create(_l: u32, _s: u32) -> Weight { | ||
| Weight::zero() | ||
| } | ||
| /// NOTE: placeholder — re-run benchmarks after merging. | ||
| fn add_to_vesting_merge(_l: u32, _s: u32) -> Weight { | ||
| Weight::zero() | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we update this one to point
weights::pallet_vesting_vesting::…(the newly generated one after you run /cmd bench)? You probably need to add it incumulus/parachains/runtimes/assets/asset-hub-westend/src/weights/mod.rstoo.