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

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

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 requested a review from gavofyork July 19, 2019 08:10
@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.

10 participants