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

Extract frame_system SignedExtensions into separate files. #6474

Merged
merged 10 commits into from
Jun 24, 2020

Conversation

tomusdrw
Copy link
Contributor

@tomusdrw tomusdrw commented Jun 22, 2020

All SignedExtensions of frame_system used to be placed in one file frame/system/src/lib.rs together with the module implementation and the tests.
The file became rather large over time and the code & tests mixed up.

The PR moves each extension to it's own module under extensions directory and moves corresponding tests there as well. The test initialisation code is extracted to mock.rs file, similar to how other pallets are structured.

I didn't change any logic, nor did it change any APIs (extensions are re-exported), it only introduced a bunch of changes necessary to make the code work with the new structure (pub(crate), etc).

Done because I want to change how weights are parametrized and realised that I'd need to add a bunch of new lines to this enormous file, so rather decided to split it first.

Polkadot Companion: paritytech/polkadot#1306

@tomusdrw tomusdrw added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Jun 22, 2020
Copy link
Member

@shawntabrizi shawntabrizi left a comment

Choose a reason for hiding this comment

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

Great!

Although might as well also move tests into a tests.rs?

@tomusdrw
Copy link
Contributor Author

@shawntabrizi tests moved as well.


/// Check for transaction mortality.
#[derive(Encode, Decode, Clone, Eq, PartialEq)]
pub struct CheckEra<T: Trait + Send + Sync>(Era, sp_std::marker::PhantomData<T>);
Copy link
Member

Choose a reason for hiding this comment

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

If we move this, can we may directly rename this?

And call it CheckMortality to make it more obvious what this is doing? I first always think that this checks the era.

transaction_validity::{
ValidTransaction, TransactionValidityError, InvalidTransaction, TransactionValidity,
},

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

TY!

@jacogr
Copy link
Contributor

jacogr commented Jun 23, 2020

This is fine - but let's not rush this through to the networks since it needs an API and extension rollout (with approvals). The signed extensions are used in there to make sense of the payload.

@tomusdrw
Copy link
Contributor Author

CC @jacogr the IDENTIFIER stays the same now, are you okay with the PR then?

Copy link
Contributor

@jacogr jacogr left a comment

Choose a reason for hiding this comment

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

All ok.

@tomusdrw tomusdrw merged commit d17396c into master Jun 24, 2020
@tomusdrw tomusdrw deleted the td-split-check branch June 24, 2020 15:01
@tomusdrw tomusdrw added A8-mergeoncegreen and removed A0-please_review Pull request needs code review. labels Jun 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants