-
Notifications
You must be signed in to change notification settings - Fork 487
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
Fix post dispatch weight #851
Fix post dispatch weight #851
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, thank you :)
.actual_weight | ||
.unwrap(); | ||
|
||
let expected_weight = base_extrinsic_weight.saturating_add(post_dispatch_weight); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implies that we are adding Substrate`s notion of base weight and Ethereum's. I don't believe these should be added in this way, as they both account for the same things (signature verification and basic accounting of fees).
It would be great if we could convince substrate not to include the extrinsic_base_weight
...
* Call post_dispatch for CheckedSignature::SelfContained * add tests * rename tests Co-authored-by: Nisheeth Barthwal <nbaztec@gmail.com>
* Call post_dispatch for CheckedSignature::SelfContained * add tests * rename tests Co-authored-by: Nisheeth Barthwal <nbaztec@gmail.com> (cherry picked from commit d3beddc)
This fixes a bug where weight was not refunded after execution, which meant that the worst-case weight (based on
gas_limit
) was accounted for in all cases. This change callsExtra::post_dispatch
which can invokeCheckWeight::post_dispatch
, which will properly refund any extra weight.Note that this only applies to
weight
accounting, the currency used for fees was already working as designed.Thanks to @nbaztec for adding tests.