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

Fees API Improvement #1623

Merged
merged 5 commits into from
Nov 27, 2023
Merged

Fees API Improvement #1623

merged 5 commits into from
Nov 27, 2023

Conversation

lemunozm
Copy link
Contributor

@lemunozm lemunozm commented Nov 24, 2023

Description

Problem

The current API provided by the traits around pallet_fees to handle fees has two negative points:

  1. In order to use it in a pallet you need to use at least 2 associated types in the Config trait. i.e:
/// Entity used to pay fees
type Fees: Fees<AccountId = Self::AccountId>;

/// Key used to get the fee value.
type ActionAFeeKey: Get<<Self::Fees as Fees>::FeeKey>;
  1. The knowledge of how to consume that fee is in the pallet when the pallet should not be aware of it. This should be a runtime concern.
/// Inside the pallet we decide when we should not do that
T::Fees::fee_to_treasury(&who, Fee::Key(T::ActionAFeeKey))?;
// or
T::Fees::fee_to_author(&who, Fee::Balance(T::ActionAFeeValue))?;

Solution

We can fix these 2 points by coding the whole fee action in different types (see PR code) under a new trait PayFee.

Pallet:

// In Config trait:
type ActionAPayFee: PayFee<T::Account>;

// When use it:
T::ActionAPayFee::pay_for(&who)?;

Runtime:

parameter_types! {
    pub const ActionAFee: Fee = Fee::Balance(10 * CFG);
}

impl pallet_example::Config for Runtime {
    // We set in one line:
    //   - How the fee is handled
    //   - The value of the fee
    type ActionAPayFee = FeeToTreasury<Fees, ActionAFee>;
    // ...
}

@lemunozm lemunozm added P2-nice-to-have Issue is worth doing. I6-refactoring Code needs refactoring. labels Nov 24, 2023
@lemunozm lemunozm self-assigned this Nov 24, 2023
@lemunozm
Copy link
Contributor Author

This PR is completed and ready for review

cdamian
cdamian previously approved these changes Nov 27, 2023
NunoAlexandre
NunoAlexandre previously approved these changes Nov 27, 2023
Copy link
Contributor

@NunoAlexandre NunoAlexandre left a comment

Choose a reason for hiding this comment

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

Sweet 🍯 👌

libs/traits/src/lib.rs Outdated Show resolved Hide resolved
@lemunozm lemunozm dismissed stale reviews from NunoAlexandre and cdamian via 75c10cc November 27, 2023 15:03
@lemunozm lemunozm enabled auto-merge (squash) November 27, 2023 15:04
Copy link
Contributor

@NunoAlexandre NunoAlexandre left a comment

Choose a reason for hiding this comment

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

One doc request but not blocking 🙏

/// This trait can be used by pallet to just pay fees without worring about
/// the value or where the fee goes.
pub trait PayFee<AccountId> {
fn pay(who: &AccountId) -> DispatchResult;
Copy link
Contributor

Choose a reason for hiding this comment

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

Given the previous discussion, I would for sure document that who is the account that's paying for the fees and not the account that is receiving them.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 on that. Would also be great to rename who to payer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! Sorry for the "auto-merge", keep in mind for a future PR where I could add this 🧠

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here it goes: 2901f0e

@lemunozm lemunozm merged commit a38a6de into main Nov 27, 2023
8 checks passed
@lemunozm lemunozm deleted the fees-api-improvement branch November 27, 2023 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I6-refactoring Code needs refactoring. P2-nice-to-have Issue is worth doing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants