feat: impl block rewards #1198

wischli · 2023-02-10T17:13:38Z

Description

Implements pallet-block-rewards.

Changes and Descriptions

Create pallet_block_rewards based on the rewards spec
- Unit tests
- Benchmarks
- Integration tests
Add to Develop runtime: Rewards should be minted
- Probably requires adding an interface for handling the token origin in <<pallet_rewards::Pallet::<T, I>> as GroupRewards>::reward_group which defaults to T::Currency::mint_into: Add interface Inflation exposing fn inflate(amount) -> DispatchResult
- Something similar for remainder handling

Configurable minting method, that is, configure the pallet with an Option<AccountId> or similar to give the account used for rewards or None to mint it

Add init migration
Benchmark pallet_block_rewards
Benchmark pallet_collator_selection
Benchmark pallet_session

Handled in a follow-up PR

Add to Altair runtime: Rewards should be pulled from Treasury
Add to Centrifuge runtime: Rewards should be minted

Type of change

New feature (non-breaking change which adds functionality)
This change requires a documentation update

How Has This Been Tested?

cargo test -p pallet-block-rewards --features runtime-benchmarks
PARA_CHAIN_SPEC=development ./scripts/init.sh benchmark pallet-block-rewards

Checklist:

I have added Rust doc comments to structs, enums, traits and functions
I have made corresponding changes to the documentation
I have performed a self-review of my code
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules
I rebased on the latest main branch

pallets/rewards/src/lib.rs

lemunozm

I really like how it looks! Good job William! 🙌🏻

Some comments/suggestions/questions below

runtime/development/src/lib.rs

pallets/block-rewards/src/lib.rs

mikiquantum

Sorry for coming a bit late to the convo. Looks good overall!

pallets/block-rewards/src/lib.rs

mikiquantum · 2023-02-15T15:50:47Z

pallets/block-rewards/src/lib.rs

+		/// Admin method to set the inflation amount for the Beneficiary used for the next epochs.
+		/// Current epoch is not affected by this call.
+		#[pallet::weight(T::WeightInfo::set_distributed_reward())]
+		pub fn set_beneficiary_reward(


I am wondering if instead of giving the exact amount here we do give the inflation_per_epoch and then internally we substract the collator reward and the rest goes to the beneficiary (if set). If I am seeing this correctly, this will avoid us from:

Having to calculate the split between those two values outside of the chain

Everytime we include a new collator, we will need to update this value, which possibly for a period of time we would be giving more than what we should.

WDYT?

We could definitely do it your way as well! I kind of went this path before 1ee21af, when it became aware that rewarding a group with amount x consumes the entire x.

Everytime we include a new collator, we will need to update this value, which possibly for a period of time we would be giving more than what we should.

The current design does not need to be reconfigured when new collators join as the configured reward is per collator, at least under the assumption that n * collator_reward + beneficiary_amount does not need to stay static.
However, I definitely see the upside in your approach. Just on a note: We would still need to store two reward amounts, otherwise we would have to hardcode the share for a collator.

yes, the collator_reward is per collator and that doesn't have to change when changing the collator size, but that new value now affects the beneficiary_amount that goes into the treasury right? That value will need to be adapted accordingly when a new size of collators is defined.
Am I understanding correctly that the beneficiary_reward is the 3% inflation rate - the collator reward? If so those two values are interconnected.
Yes, in my proposal we still have to store 2 values.

Yes that is correct. Let's summarize: I will change beneficiary_reward to denote the total reward per epoch instead. From that, we subtract the rewards for all collators and the rest

is minted in the Treasury for Centrifuge chain

stays in the Treasury for Altair, e.g. is not moved anywhere

I think so, that means that the beneficiary_reward, if set, has to be at least the same as the collator_reward * number of collators at that point in time.
And for Altair, we basically do not set it, and the logic should be able to only mint/move the collator_reward without interacting with the Zero value of the beneficiary_reward.

Fixed in 8b8c40f. Not sure if I should rename Beneficiary to Treasury.

lemunozm

It's ready for me! 🚀

Good job @wischli !!

pallets/block-rewards/src/lib.rs

mustermeiszer

Niiiiiiice!!

Migrations looks good, code looks good, tests look good, integrations test are sick!

The only thing before merging this one from my side would be to check miguels comment and wait for #1233

libs/types/src/ids.rs

pallets/block-rewards/src/lib.rs

pallets/block-rewards/Cargo.toml

pallets/block-rewards/src/migrations.rs

mustermeiszer · 2023-03-24T10:43:20Z

pallets/block-rewards/src/migrations.rs

+					.ok();
+				weight.saturating_accrue(T::DbWeight::get().reads_writes(6, 6));
+			}
+			Pallet::<T>::current_storage_version().put::<Pallet<T>>();


Is this substrate magic? Where is the version defined?

The function itself is part of the GetStorageVersion trait. The pallet's storage version current_storage_version is in scope of the frame_support::pallet magic.

So anytime we bump a pallet's storage version we still need to do a migration to have the on_chain_storage_version and current_storage_version aligned.

mustermeiszer · 2023-03-24T10:45:25Z

runtime/development/src/lib.rs

+			pallet_rewards::Pallet::<Runtime, pallet_rewards::Instance1>::list_currencies(&account_id)
+			.into_iter().chain(
+				pallet_rewards::Pallet::<Runtime, pallet_rewards::Instance2>::list_currencies(&account_id).into_iter()
+			).collect()


Is list_currencies a trait method?

Then we can impl that for tuples.

Yes it is! Should this be handled in this PR or in a follow-up one?

Follow up. Just a clean up

mustermeiszer · 2023-03-24T10:54:58Z

pallets/block-rewards/src/lib.rs

+			collator_reward_per_session: T::Balance,
+		) -> DispatchResult {
+			T::AdminOrigin::ensure_origin(origin)?;
+


I think this is not needed, but a good sanity check maybe as we also do this check in set_total_reward. Wdyt @wischli or is the check in set_total_reward otherwise a safety mesaure?

do_advance: Already does check let total_reward = min(num_collators*reward_collator, total_reward)

lemunozm · 2023-03-27T15:20:05Z

pallets/block-rewards/src/lib.rs

+
+		/// The identifier of the artificial block rewards currency which is minted and burned for collators.
+		#[pallet::constant]
+		type StakeCurrencyId: Get<CfgCurrencyId>;


Can CfgCurrencyId be identified by Currency::AssetId?

I mean, the pallet can works without knowing that the CurrencyId is exactly a CfgCurrencyId

Yes, we have to add an AssetId associated type to the Config then. Will do this tomorrow! Good catch.

No hurry! Maybe a CurrencyIdOf<T> using the CurrencyId from Rewards traits or from Currency trait saves you from another Config parameter

Since we need to specify the CurrencyId for the Rewards trait, I don't see how we get around adding a CurrencyId parameter to the pallet_block_rewards::Config.

lemunozm

LGTM! Let's get this merged soon!

A couple of minor questions.

lemunozm · 2023-04-14T11:39:49Z

runtime/development/src/lib.rs

 >;

+type UpgradeDev1020 =
+	pallet_block_rewards::migrations::InitBlockRewards<Runtime, CollatorRewards, TotalRewards>;


Why is this migration needed for development?

For the demo chain and to enable testing the migration locally before we add it to Altair and Centrifuge later.

Ok, so as a norm, we should always create migrations for any breaking change in development, right?

I would say so, yes.

runtime/development/src/weights/pallet_restricted_tokens.rs

lemunozm

Re approval! 🚀

mustermeiszer

Re-approve

wischli added 5 commits February 6, 2023 10:17

chore: init block rewards from liquidity rewards clone

b9bd2c4

feat: adjust extrinsics, add SessionManager

a69bb5e

refactor: use OneSessionHandler, handle minting, add docs

a2f9a79

chore: remove admin deposit/withdraw_stake

1c81915

chore: impl BlockRewards for dev runtime

c4dbe47

wischli self-assigned this Feb 10, 2023

wischli added 4 commits February 13, 2023 14:14

feat: make reward source configurable

795036d

Merge remote-tracking branch 'origin' into feat/impl-block-rewards

664eee6

feat: mint remainder

020ec78

feat: finalize session handling

ccae5c6

lemunozm reviewed Feb 14, 2023

View reviewed changes

pallets/rewards/src/lib.rs Outdated Show resolved Hide resolved

lemunozm reviewed Feb 14, 2023

View reviewed changes

pallets/rewards/src/lib.rs Outdated Show resolved Hide resolved

lemunozm reviewed Feb 14, 2023

View reviewed changes

wischli added 7 commits February 14, 2023 15:05

feat: init rewards at genesis

a99a583

fix: collator, treasury reward split

1ee21af

refactor: use OnUnbalanced instead of PalletId

257308d

fix: beneficiary reward currency

bdb6d12

chore: remove dummy bench, mock, tests

97292e2

fix: session + collator selection benches

4c48350

refactor: hardcode many ATs

6fa7c4a

mikiquantum reviewed Feb 15, 2023

View reviewed changes

wischli added 3 commits February 16, 2023 14:25

refactor: reward source v2

0d73cf3

refactor: issuance of rewards

a5ac903

tests: apply RewardIssuance

910139a

wischli mentioned this pull request Feb 17, 2023

feat: reward issuance interface #1209

Merged

8 tasks

lemunozm previously approved these changes Mar 21, 2023

View reviewed changes

pallets/block-rewards/src/lib.rs Outdated Show resolved Hide resolved

mustermeiszer previously approved these changes Mar 24, 2023

View reviewed changes

wischli added 4 commits March 27, 2023 11:05

feat: add check to set_collator_rewards

27e4653

chore: update CODEOWNERS

7223a31

refactor: stake amt, group id via associated type

02060eb

chore: move dev rewards config closer to prod

5be160c

wischli dismissed stale reviews from mustermeiszer and lemunozm via 5be160c March 27, 2023 13:20

lemunozm reviewed Mar 27, 2023

View reviewed changes

wischli added 4 commits March 28, 2023 09:10

refactor: abstract CfgCurrencyId

ecd49de

Merge remote-tracking branch 'origin/main' into feat/impl-block-rewards

8650cc8

Merge remote-tracking branch 'origin/main' into feat/impl-block-rewards

69de861

fix: bench errors after rebase

97a0dbe

wischli requested a review from lemunozm April 14, 2023 11:33

lemunozm previously approved these changes Apr 14, 2023

View reviewed changes

fix: dev try-runtime

5a3a6ad

wischli dismissed lemunozm’s stale review via 5a3a6ad April 14, 2023 14:42

lemunozm previously approved these changes Apr 14, 2023

View reviewed changes

lemunozm mentioned this pull request Apr 14, 2023

Loans: Add multiple triggers for write-off #1314

Merged

2 tasks

fix: benchmarks

572d9e2

wischli dismissed lemunozm’s stale review via 572d9e2 April 17, 2023 06:27

wischli requested review from lemunozm and mustermeiszer April 17, 2023 06:27

Merge branch 'main' into feat/impl-block-rewards

cbb10b1

lemunozm approved these changes Apr 17, 2023

View reviewed changes

mustermeiszer approved these changes Apr 17, 2023

View reviewed changes

wischli enabled auto-merge (squash) April 17, 2023 07:49

hieronx approved these changes Apr 17, 2023

View reviewed changes

wischli merged commit c068e8b into main Apr 17, 2023

wischli mentioned this pull request May 11, 2023

feat: add block rewards to altair and centrifuge runtimes #1342

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: impl block rewards #1198

feat: impl block rewards #1198

wischli commented Feb 10, 2023 •

edited

Loading

lemunozm left a comment

mikiquantum left a comment

mikiquantum Feb 15, 2023

wischli Feb 15, 2023

mikiquantum Feb 15, 2023

wischli Feb 15, 2023 •

edited

Loading

mikiquantum Feb 15, 2023

wischli Feb 17, 2023

lemunozm left a comment

mustermeiszer left a comment

mustermeiszer Mar 24, 2023

wischli Mar 27, 2023

mustermeiszer Mar 24, 2023

wischli Mar 27, 2023

mustermeiszer Mar 27, 2023

mustermeiszer Mar 27, 2023

mustermeiszer Mar 24, 2023

lemunozm Mar 27, 2023 •

edited

Loading

lemunozm Mar 27, 2023

wischli Mar 27, 2023 •

edited

Loading

lemunozm Mar 27, 2023 •

edited

Loading

wischli Mar 28, 2023

wischli Mar 28, 2023

lemunozm left a comment

lemunozm Apr 14, 2023

wischli Apr 14, 2023

lemunozm Apr 14, 2023

wischli Apr 14, 2023

lemunozm left a comment

mustermeiszer left a comment

feat: impl block rewards #1198

feat: impl block rewards #1198

Conversation

wischli commented Feb 10, 2023 • edited Loading

Description

Changes and Descriptions

Handled in a follow-up PR

Type of change

How Has This Been Tested?

Checklist:

lemunozm left a comment

Choose a reason for hiding this comment

mikiquantum 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

wischli Feb 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemunozm left a comment

Choose a reason for hiding this comment

mustermeiszer 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

lemunozm Mar 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wischli Mar 27, 2023 • edited Loading

Choose a reason for hiding this comment

lemunozm Mar 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemunozm 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

lemunozm left a comment

Choose a reason for hiding this comment

mustermeiszer left a comment

Choose a reason for hiding this comment

wischli commented Feb 10, 2023 •

edited

Loading

wischli Feb 15, 2023 •

edited

Loading

lemunozm Mar 27, 2023 •

edited

Loading

wischli Mar 27, 2023 •

edited

Loading

lemunozm Mar 27, 2023 •

edited

Loading