Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Transaction Fee Multiplier #2854

Merged
merged 77 commits into from
Jul 19, 2019
Merged

Transaction Fee Multiplier #2854

merged 77 commits into from
Jul 19, 2019

Conversation

4meta5
Copy link
Contributor

@4meta5 4meta5 commented Jun 12, 2019

Implementation of the slow fee mechanism -- required some changes after merging #2799

addresses #2430

TODO:

  • clean up and finish make_payment in srml/balances
  • test that the block weight limit is enforced (like here)
  • test correct fee calculation for a few transactions

cc @kianenigma

@parity-cla-bot
Copy link

It looks like @4meta5 signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

1 similar comment
@parity-cla-bot
Copy link

It looks like @4meta5 signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

@kianenigma
Copy link
Contributor

Ok just had an idea of how it can be (easily) more user-friendly:

  • Make the assumption: since Balances is the module responsible for deducting fees, it is also responsible for converting weights to fees.
  • Balance trait will accept a function that uses the Convert trait from Weight to Balance:
pub trait Trait<I: Instance = DefaultInstance>: system::Trait {
	/// The balance of an account.
	type Balance: Parameter + Member + SimpleArithmetic + Codec + Default + Copy +
		MaybeSerializeDebug + From<Self::BlockNumber>;

+       type WeightToFee: Convert<Weight, Self::Balance>;
        ....
}
  • Then:
fn make_payment(transactor: &T::AccountId, weight: Weight) -> Result {
+		let transaction_fee = T::WeightToFee::convert(weight);
		let imbalance = Self::withdraw(
			transactor,
			transaction_fee.into(),
			WithdrawReason::TransactionPayment,
			ExistenceRequirement::KeepAlive
		)?;
		T::TransactionPayment::on_unbalanced(imbalance);
		Ok(())
	}
  • node/runtime will provide an implementation for this:
impl balances::Trait for Runtime {
	type Balance = Balance;
        
+       type WeightToFee = SomeStructThatImplementsIt;
	
        type OnFreeBalanceZero = ((Staking, Contract), Session);
	type OnNewAccount = Indices;
	type Event = Event;
	type TransactionPayment = ();
	type DustRemoval = ();
	type TransferPayment = ();
}

cc @bkchr

node/runtime/src/lib.rs Outdated Show resolved Hide resolved
node/runtime/src/lib.rs Outdated Show resolved Hide resolved
node/runtime/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@gavofyork gavofyork left a comment

Choose a reason for hiding this comment

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

Didn't see that stuff in srml/executive. That needs to be sorted before this can be merged.

use system;
pub use balances::Call as balancesCall;
pub use system::Call as systemCall;
use node_runtime::impls::WeightMultiplierUpdateHandler;
Copy link
Member

Choose a reason for hiding this comment

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

Invalid. The point of this is to test executive. The test runtime exists as a minimal runtime designed to facilitate that in a controlled fashion. This should not be testing the fully-featured substrate-node runtime, nor should it be pulling in anything from that set of crates.

Copy link
Contributor

Choose a reason for hiding this comment

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

Will take this into account but do note that this PR will probably have to be closed and a portion of it need to be ported to into your PR as a signed extension.

@kianenigma
Copy link
Contributor

@gavofyork done + brought back the original substrate base/byte fee stuff. Should be okay to be merged and used with your PR. Still contains some historical MakePayment() stuff that will be removed in #3102.

@gavofyork gavofyork mentioned this pull request Jul 16, 2019
11 tasks
///
/// This will perform a saturated `weight + weight * self.0`.
pub fn apply_to(&self, weight: Weight) -> Weight {
let inner = self.0;
Copy link
Member

Choose a reason for hiding this comment

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

we really shouldn't be mucking around with the inner member of a Fixed64 in here and depending on DIV being public API (it shouldn't be). operations should really be defined purely in terms of a proper public API of Fixed64.

/// Apply the inner Fixed64 as a weight multiplier to a weight value.
///
/// This will perform a saturated `weight + weight * self.0`.
pub fn apply_to(&self, weight: Weight) -> Weight {
Copy link
Member

Choose a reason for hiding this comment

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

should just be called multiply_accumulate and moved into Fixed64.

Copy link
Member

@gavofyork gavofyork left a comment

Choose a reason for hiding this comment

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

Bit of code shifting needed to clean up FeeMutiplier but looks good otherwise.

@gavofyork gavofyork added this to the 2.0-k milestone Jul 19, 2019
@kianenigma kianenigma merged commit afa5830 into master Jul 19, 2019
@kianenigma kianenigma deleted the amar/tx-fee-multiplier branch July 19, 2019 12:21
///
/// Note that this might be lossy.
pub fn from_rational(n: i64, d: u64) -> Self {
Self((n as i128 * DIV as i128 / (d as i128).max(1)).try_into().unwrap_or(Bounded::max_value()))
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't checked_div be used here to avoid divide by zero issue?

Copy link
Contributor

Choose a reason for hiding this comment

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

.max(0)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants