Skip to content
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

feat: impl block rewards #1198

Merged
merged 83 commits into from
Apr 17, 2023
Merged

feat: impl block rewards #1198

merged 83 commits into from
Apr 17, 2023

Conversation

wischli
Copy link
Contributor

@wischli wischli commented Feb 10, 2023

Description

Implements pallet-block-rewards.

Fixes #1000

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

@wischli wischli added I5-documentation Documentation needs fixing or improving. Q3-medium Can be done with good experience in the language, but little knowledge of the codebase. I4-tests Test needs fixing or improving. I8-enhancement An additional feature. crcl-protocol Circle protocol related. labels Feb 10, 2023
@wischli wischli self-assigned this Feb 10, 2023
Copy link
Contributor

@lemunozm lemunozm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Some comments/suggestions/questions below

runtime/development/src/lib.rs Outdated Show resolved Hide resolved
runtime/development/src/lib.rs Outdated Show resolved Hide resolved
runtime/development/src/lib.rs Outdated Show resolved Hide resolved
runtime/development/src/lib.rs Outdated Show resolved Hide resolved
pallets/block-rewards/src/lib.rs Outdated Show resolved Hide resolved
pallets/block-rewards/src/lib.rs Show resolved Hide resolved
pallets/block-rewards/src/lib.rs Show resolved Hide resolved
pallets/block-rewards/src/lib.rs Show resolved Hide resolved
Copy link
Contributor

@mikiquantum mikiquantum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

pallets/block-rewards/src/lib.rs Outdated Show resolved Hide resolved
pallets/block-rewards/src/lib.rs Show resolved Hide resolved
pallets/block-rewards/src/lib.rs Outdated Show resolved Hide resolved
/// 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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@wischli wischli Feb 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@wischli wischli mentioned this pull request Feb 17, 2023
8 tasks
lemunozm
lemunozm previously approved these changes Mar 21, 2023
Copy link
Contributor

@lemunozm lemunozm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's ready for me! 🚀

Good job @wischli !!

pallets/block-rewards/src/lib.rs Outdated Show resolved Hide resolved
mustermeiszer
mustermeiszer previously approved these changes Mar 24, 2023
Copy link
Collaborator

@mustermeiszer mustermeiszer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
pallets/block-rewards/src/lib.rs Outdated Show resolved Hide resolved
pallets/block-rewards/Cargo.toml Show resolved Hide resolved
pallets/block-rewards/src/migrations.rs Show resolved Hide resolved
.ok();
weight.saturating_accrue(T::DbWeight::get().reads_writes(6, 6));
}
Pallet::<T>::current_storage_version().put::<Pallet<T>>();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this substrate magic? Where is the version defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +2055 to +2058
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()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is list_currencies a trait method?

Then we can impl that for tuples.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow up. Just a clean up

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow up. Just a clean up

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

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

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

@lemunozm lemunozm Mar 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can CfgCurrencyId be identified by Currency::AssetId?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@wischli wischli Mar 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@lemunozm lemunozm Mar 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@wischli wischli requested a review from lemunozm April 14, 2023 11:33
lemunozm
lemunozm previously approved these changes Apr 14, 2023
Copy link
Contributor

@lemunozm lemunozm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Let's get this merged soon!

A couple of minor questions.

>;

type UpgradeDev1020 =
pallet_block_rewards::migrations::InitBlockRewards<Runtime, CollatorRewards, TotalRewards>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this migration needed for development?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say so, yes.

lemunozm
lemunozm previously approved these changes Apr 14, 2023
Copy link
Contributor

@lemunozm lemunozm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re approval! 🚀

Copy link
Collaborator

@mustermeiszer mustermeiszer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approve

@wischli wischli enabled auto-merge (squash) April 17, 2023 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crcl-protocol Circle protocol related. I4-tests Test needs fixing or improving. I5-documentation Documentation needs fixing or improving. I8-enhancement An additional feature. Q3-medium Can be done with good experience in the language, but little knowledge of the codebase.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rewards: pallet-block-rewards implementation
6 participants