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

Author reward destination through inherent, fees, tips, and treasury #11

Merged
merged 16 commits into from
Apr 2, 2024

Conversation

gui1117
Copy link
Contributor

@gui1117 gui1117 commented Mar 29, 2024

related issue: #3
Ready for review.

  • New pallet with an inherent so that block author can set a destination account for its reward (fees + tips).
  • Inherent is mandatory: block author must set a destination account, debatable.
  • Treasury pallet is added and very basic configuration.
  • Split between author and treasury can be readjusted later.
  • Block author ultimately choose the reward destination with the environment variable DROPIT_AUTHOR_REWARD_DEST and format is SS58.
  • zombie test net may need more adjustment so every collator adds this environment variable but it doesn't work on master. We can do in a follow up.

@gui1117 gui1117 changed the title WIP: author reward destination through inherent, fees, tips, and treasury Author reward destination through inherent, fees, tips, and treasury Mar 30, 2024
@gui1117 gui1117 marked this pull request as ready for review March 30, 2024 11:32
@shawntabrizi
Copy link
Contributor

zombie test net may need more adjustment so every collator adds this environment variable but it doesn't work on master. We can do in a follow up.

This comment is out of date? Zombienet looks fine with your last commit

runtime/src/common.rs Outdated Show resolved Hide resolved
runtime/src/lib.rs Outdated Show resolved Hide resolved
pub const ProposalBond: Permill = Permill::from_percent(5);
pub const ProposalBondMinimum: Balance = 0;
pub const ProposalBondMaximum: Option<Balance> = Option::None;
pub const SpendPeriod: BlockNumber = 24 * DAYS;
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, actually we might need to create our own treasury pallet which doesn't have these kinds of concepts, since the chain won't necessarily be producing a block every 6 seconds

Copy link
Contributor

Choose a reason for hiding this comment

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

For now, we should set this very low, like 100 blocks maybe

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, just remembering, we need to make our own pallet which is actually connected to the relay chain block number

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally feature should be added to treasury

Maybe SpendPeriod type should be able to take either a block number or a relay block number. I will look into it

Copy link
Contributor

@shawntabrizi shawntabrizi left a comment

Choose a reason for hiding this comment

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

Some nits. Probably need to think a little more about treasury, but can be done in a follow up PR.

Once test is fixed, happy to merge.

#[pallet::config]
pub trait Config: frame_system::Config {
/// The type `Self::AccountId` but with additional constraint.
type AccountIdType: IsType<<Self as frame_system::Config>::AccountId> + From<InherentType>;

Choose a reason for hiding this comment

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

/// Set the author reward destination for the block.
#[pallet::call_index(0)]
#[pallet::weight((
T::DbWeight::get().writes(1),

Choose a reason for hiding this comment

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

fwiw this is not really POV friendly as types like RuntimeDbWeight and such don't track PoV at all.

The solution for now is to not use this "manual" weighing in code and only write and run benchmarks.

I can't find it, but we recently re-discovered this and made an issue to solve it or want users about it better cc @ggwpez

Copy link
Contributor

Choose a reason for hiding this comment

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

or to provide a weight metered DB: polkadot-fellows/RFCs#49

Copy link
Contributor

Choose a reason for hiding this comment

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

But anyway we will do becnhmarks in a final sprint of the runtime

@shawntabrizi
Copy link
Contributor

happy to merge

@gui1117
Copy link
Contributor Author

gui1117 commented Apr 2, 2024

ok to merge.

treasury needs more work,
I started benchmarking here but struggle: #14

@shawntabrizi shawntabrizi merged commit 8affdaf into master Apr 2, 2024
1 check passed
@shawntabrizi shawntabrizi deleted the gui-authorship-treasury branch April 2, 2024 01:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants