Refactor SignedExtension #5540

athei · 2020-04-06T10:46:03Z

polkadot companion: paritytech/polkadot#979

This PR refactors the trait SignedExtension. The following changes where applied:

Move the `DispatchInfo` associated type to `Dispatchable::Info`

This information logically belongs to the Dispatchable. Not having it there requires consuming types to duplicate this as an associated type (SignedExtension, Applyable).

Bound `Call: Dispatchable`

This is a requirement for the first change. Because the DispatchInfo type is now an associated type on Dispatchable.

Pass `Dispatchable::PostInfo` to `SignedExtension::post_dispatch`

This is a followup of #5458 that passes this new information to the SignedExtension where it can then be used to accomplish weight refund (in a followup PR).

Pass `Dispatchable::Info` by reference

There was no apparent reason (or is there one?) for having DispatchInfo to be cloned into every SignedExtension.

parity-cla-bot · 2020-04-06T10:46:07Z

It looks like @athei signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

tomusdrw

on the right track, but we need to reduce boilerplate

frame/balances/src/tests.rs

frame/balances/src/tests_composite.rs

frame/balances/src/tests_local.rs

frame/executive/src/lib.rs

frame/staking/src/lib.rs

frame/transaction-payment/src/lib.rs

primitives/runtime/src/generic/checked_extrinsic.rs

primitives/runtime/src/traits.rs

frame/balances/src/tests.rs

frame/balances/src/tests_composite.rs

frame/balances/src/tests_local.rs

frame/example/src/lib.rs

frame/executive/src/lib.rs

frame/staking/src/lib.rs

primitives/runtime/src/traits.rs

kianenigma

I realised something:

I don't think it actually makes sense for dispatchInfo to be generic at the moment. Maybe we can make it generic, but currently it isn't.
otherwise, it doesn't even make sense for it to be generic as it is since it is kinda hard-coded in frame-support::dispatch. Your weight annotation must return weight + pays_fee + class and then they get bundled into this type DispatchInfo.

Yes, you can use any type to compute these 3, but they are fixed themselves. From this PoV I think this work needs to be refactored again.

I recommend merging this PR leniently just to unblock weight refund and I can make a follow up asap and my goal would be actually to either rethink the whole process or make it un-generic, as it used to be.

athei · 2020-04-06T15:25:51Z

An alternative approach would be that I pass the Dispatchable::PostInfo as an additional associated type of SignedExtension. Then the refactor can happen after getting the refund merged. However, this would clash with how we want to do it eventually (move associated types way from SignedExtension. @tomusdrw what do you think?

tomusdrw · 2020-04-06T15:29:40Z

I don't think it actually makes sense for dispatchInfo to be generic at the moment. Maybe we can make it generic, but currently it isn't.

Yes, I think @athei is fully aware of that, but it making it a fixed type would require moving Dispatchable to sp_runtime (which might be another big task).

An alternative approach would be that I pass the Dispatchable::PostInfo as an additional associated type of SignedExtension. Then the refactor can happen after getting the refund merged. However, this would clash with how we want to do it eventually (move associated types way from SignedExtension. @tomusdrw what do you think?

I think this PR looks good and is a bit nicer than current approach, so I think we should merge it as is (especially that it unlocks stuff). We can further change it in the future if needed.

athei · 2020-04-06T15:42:15Z

Yes, I think @athei is fully aware of that, but it making it a fixed type would require moving Dispatchable to sp_runtime (which might be another big task).

Dispatchableis in sp_runtime. Do you mean FRAME?

tomusdrw · 2020-04-06T15:44:05Z

Dispatchableis in sp_runtime. Do you mean FRAME?

Ugh, yes. So it's now generic, since weight is frame-specific, right?

athei · 2020-04-06T15:45:18Z

Yes Dispatchable is generic since #5458.

* Move DispatchInfo Associated type to Dispatchable * Bound Call: Dispatchable * Pass PostDispatchInfo to post_dispatch * Pass DispatchInfo by reference to avoid clones

Co-Authored-By: Tomasz Drwięga <tomusdrw@users.noreply.github.com>

Co-Authored-By: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

athei · 2020-04-07T07:53:35Z

I think this PR looks good and is a bit nicer than current approach, so I think we should merge it as is (especially that it unlocks stuff). We can further change it in the future if needed.

That would be nice. I think I addressed all raised concerns now. Praying to CI gods.

frame/balances/src/tests_composite.rs

primitives/runtime/src/traits.rs

Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com>

athei · 2020-04-07T13:21:47Z

I added the Polkadot companion and tested it against this PR locally.

…adot#986 (paritytech#982) * Companion PR to paritytech#5560 * Set priorities. * Update substrate. * Fix tests. * Update Substrate * Companion of SignedExtension refactor (paritytech#5540) Co-authored-by: Tomasz Drwięga <tomasz@parity.io> Co-authored-by: Alexander Theißen <alexander.theissen@parity.io>

athei added the A0-please_review Pull request needs code review. label Apr 6, 2020

athei requested review from tomusdrw and kianenigma April 6, 2020 10:46

athei requested review from pepyakin and gui1117 as code owners April 6, 2020 10:46

paritytech-ci added the B2-breaksapi label Apr 6, 2020

tomusdrw reviewed Apr 6, 2020

View reviewed changes

kianenigma reviewed Apr 6, 2020

View reviewed changes

primitives/runtime/src/traits.rs Show resolved Hide resolved

kianenigma reviewed Apr 6, 2020

View reviewed changes

athei and others added 7 commits April 7, 2020 09:50

Refactor SignedExtension

8354714

* Move DispatchInfo Associated type to Dispatchable * Bound Call: Dispatchable * Pass PostDispatchInfo to post_dispatch * Pass DispatchInfo by reference to avoid clones

Whitespace fix

3f3a756

Co-Authored-By: Tomasz Drwięga <tomusdrw@users.noreply.github.com>

Style changes from code review

73456d3

Co-Authored-By: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

Only decalre in test mod to remove warning

b3bb925

Deduplicate Call definition

020c422

Bound frame_system::trait::Call by Dispatchable

839b223

Introduce DispatchInfoOf type alias

211334a

athei force-pushed the at-refactor-signed-extension branch from dabb4d3 to 211334a Compare April 7, 2020 07:51

bkchr reviewed Apr 7, 2020

View reviewed changes

frame/balances/src/tests_composite.rs Outdated Show resolved Hide resolved

primitives/runtime/src/traits.rs Show resolved Hide resolved

athei mentioned this pull request Apr 7, 2020

Companion of SignedExtension refactor (#5540) paritytech/polkadot#979

Closed

Whitespace fix from review

8037384

Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com>

kianenigma approved these changes Apr 8, 2020

View reviewed changes

gavofyork merged commit 78cb165 into master Apr 8, 2020

gavofyork deleted the at-refactor-signed-extension branch April 8, 2020 09:12

athei mentioned this pull request Apr 8, 2020

Introduce weight refund #5584

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor SignedExtension #5540

Refactor SignedExtension #5540

athei commented Apr 6, 2020 •

edited

Loading

parity-cla-bot commented Apr 6, 2020

tomusdrw left a comment

kianenigma left a comment

athei commented Apr 6, 2020

tomusdrw commented Apr 6, 2020 •

edited by athei

Loading

athei commented Apr 6, 2020

tomusdrw commented Apr 6, 2020

athei commented Apr 6, 2020

athei commented Apr 7, 2020 •

edited

Loading

athei commented Apr 7, 2020

Refactor SignedExtension #5540

Refactor SignedExtension #5540

Conversation

athei commented Apr 6, 2020 • edited Loading

Move the DispatchInfo associated type to Dispatchable::Info

Bound Call: Dispatchable

Pass Dispatchable::PostInfo to SignedExtension::post_dispatch

Pass Dispatchable::Info by reference

parity-cla-bot commented Apr 6, 2020

tomusdrw left a comment

Choose a reason for hiding this comment

kianenigma left a comment

Choose a reason for hiding this comment

athei commented Apr 6, 2020

tomusdrw commented Apr 6, 2020 • edited by athei Loading

athei commented Apr 6, 2020

tomusdrw commented Apr 6, 2020

athei commented Apr 6, 2020

athei commented Apr 7, 2020 • edited Loading

athei commented Apr 7, 2020

athei commented Apr 6, 2020 •

edited

Loading

Move the `DispatchInfo` associated type to `Dispatchable::Info`

Bound `Call: Dispatchable`

Pass `Dispatchable::PostInfo` to `SignedExtension::post_dispatch`

Pass `Dispatchable::Info` by reference

tomusdrw commented Apr 6, 2020 •

edited by athei

Loading

athei commented Apr 7, 2020 •

edited

Loading