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

Conversation

@pepyakin
Copy link
Contributor

This PR is a draft of the changes for weight refunds (aka dynamic weights, so fixes #3730 if merged).

This is not yet ready for the code related discussion, although design related feedback is welcome.

@pepyakin pepyakin added the A3-in_progress Pull request is in progress. No review needed at this stage. label Feb 24, 2020
@pepyakin pepyakin requested a review from kianenigma February 24, 2020 13:35
) -> Result<Self::Pre, TransactionValidityError> {
let tip = self.0;
let fee = Self::compute_fee(len as u32, info, tip);
Self::withdraw_fee(who, tip, fee).map_err(|()| InvalidTransaction::Payment.into())
Copy link
Contributor

Choose a reason for hiding this comment

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

Let see if this is safe:

  • If I panic within the code of the call itself, we have already charged the fee. That is good.
  • But for example if the fee was meant to go to authors and treasury, we don't use the imbalance and it is just dropped, so we burn it. Is this desired? Or maybe it is addressed further down in the code. But we should def. discuss this scenario.

Copy link
Contributor

Choose a reason for hiding this comment

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

@thiolliere actually, you are also changing this code so pinging you

/// Dispatch a given `T::Call` with a given origin.
///
/// Returns how much weight was unspent.
pub fn dispatch_call(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pub fn dispatch_call(
pub fn note_dispatch_call(

There should really be an emphasis on the fact that this some bookkeeping for you. Chose some other name if you want, but dispatch_call is too simple.

Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

I like this idea all in all.

  • What we need to make sure is that it is safe regards to fees.
  • The only potential huge change to it is some refactors; This logic (specially the function note_dispatch_call) can live in either system, transaction-payment, or a new pseudo-executive module. But that is not a huge concern for now.

/// Total weight for all extrinsics put together, for the current block.
AllExtrinsicsWeight: Option<Weight>;

/// The weight taken by the current executing disptachable.
Copy link
Member

Choose a reason for hiding this comment

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

typo: disptachable

fn post_dispatch(_pre: Self::Pre, info: Self::DispatchInfo, _len: usize) {
let spent = CurrentDispatchableWeight::get().unwrap_or(info.weight);
let reserved = info.weight;
let unspent = spent - reserved;
Copy link
Member

Choose a reason for hiding this comment

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

I think a comment why unchecked math is OK here is in order.

@pepyakin
Copy link
Contributor Author

pepyakin commented Mar 3, 2020

As a status update on this, there is a problem with how to introduce a global central dispatch point.

@pepyakin pepyakin mentioned this pull request Mar 4, 2020
@athei athei self-assigned this Mar 16, 2020
@athei athei force-pushed the ser-refund-weight branch from af10334 to 9a2ceef Compare March 16, 2020 15:55
@athei
Copy link
Member

athei commented Mar 16, 2020

Rebased onto current master and resolved conflicts with it. Removed some unrelated style changes that added to the noise of this PR.

Next steps will be to integrate it with the changes in #5130 as soon as there is some confidence on how the GCD will manifest.

@athei
Copy link
Member

athei commented Apr 8, 2020

Closed in favor of #5584

@athei athei closed this Apr 8, 2020
@athei athei deleted the ser-refund-weight branch September 9, 2021 08:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A3-in_progress Pull request is in progress. No review needed at this stage.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Proposal: Allow Dynamic Weights

4 participants