From 72b5cd858c145c67e353aac0e98d1b945777263d Mon Sep 17 00:00:00 2001 From: snd Date: Wed, 11 Jul 2018 21:12:22 +0200 Subject: [PATCH] Issue 212 - refactor Checkable trait to be more generic (#287) * runtime: refactor Checkable and BlindCheckable traits * fix impl BlindCheckable for Extrinsic * fix impl Checkable for TestXt * fix impl Checkable for UncheckedExtrinsic * fix tabs * add ::Address to system::Trait since its no longer in Checkable trait * replace tab by space in comment * replace occurences of Checkable::check with ::check_with * tx-pool: replace CheckedIntrinsic type alias since it now would require type param * make more uses of Checkable compile * adapt Executive impl to new Checkable trait * fix that CheckedExtrinsic takes AccountId not Address as first type param * Checkable trait: return error again since it's required in some cases * Checkable: improve docstrings * consistent punctuation and capitalization in docstrings * Ctx -> Context addresses https://github.com/paritytech/polkadot/pull/287#discussion_r200956240 * reduce trait bounds for impl Checkable for TestXt addresses https://github.com/paritytech/polkadot/pull/287#discussion_r200839303 * use ::Checked addresses https://github.com/paritytech/polkadot/pull/287#discussion_r200955165 * Revert "add ::Address to system::Trait since its no longer in Checkable trait" This reverts commit 02eb103015b833c995c9f9067aac2542bb7ce5ea. * runtime/executive: properly fix that Address no longer in Checkable * return `Result` from `Checkable::check` --- substrate/polkadot/runtime/src/utils.rs | 2 +- .../polkadot/transaction-pool/src/lib.rs | 4 +-- .../substrate/runtime/council/src/lib.rs | 2 +- .../substrate/runtime/executive/src/lib.rs | 9 ++--- .../runtime/primitives/src/generic.rs | 13 ++----- .../runtime/primitives/src/testing.rs | 7 ++-- .../runtime/primitives/src/traits.rs | 35 ++++++++++--------- substrate/substrate/test-runtime/src/lib.rs | 4 --- 8 files changed, 32 insertions(+), 44 deletions(-) diff --git a/substrate/polkadot/runtime/src/utils.rs b/substrate/polkadot/runtime/src/utils.rs index 4c16e215ba..1531590ff5 100644 --- a/substrate/polkadot/runtime/src/utils.rs +++ b/substrate/polkadot/runtime/src/utils.rs @@ -47,5 +47,5 @@ pub fn inherent_extrinsics(timestamp: ::primitives::Timestamp, parachain_heads: /// Checks an unchecked extrinsic for validity. pub fn check_extrinsic(xt: UncheckedExtrinsic) -> bool { - xt.check(Staking::lookup).is_ok() + xt.check_with(Staking::lookup).is_ok() } diff --git a/substrate/polkadot/transaction-pool/src/lib.rs b/substrate/polkadot/transaction-pool/src/lib.rs index 6e0ec39d77..a553afb94c 100644 --- a/substrate/polkadot/transaction-pool/src/lib.rs +++ b/substrate/polkadot/transaction-pool/src/lib.rs @@ -55,7 +55,7 @@ pub use extrinsic_pool::txpool::{Options, Status, LightStatus, VerifiedTransacti pub use error::{Error, ErrorKind, Result}; /// Type alias for convenience. -pub type CheckedExtrinsic = ::Checked; +pub type CheckedExtrinsic = std::result::Result>>::Checked; /// A verified transaction which should be includable and non-inherent. #[derive(Clone, Debug)] @@ -281,7 +281,7 @@ impl<'a, A> txpool::Verifier for Verifier<'a, A> where } let (encoded_size, hash) = uxt.using_encoded(|e| (e.len(), BlakeTwo256::hash(e))); - let inner = match uxt.clone().check(|a| self.lookup(a)) { + let inner = match uxt.clone().check_with(|a| self.lookup(a)) { Ok(xt) => Some(xt), // keep the transaction around in the future pool and attempt to promote it later. Err(Self::NO_ACCOUNT) => None, diff --git a/substrate/substrate/runtime/council/src/lib.rs b/substrate/substrate/runtime/council/src/lib.rs index a40a64b27b..e6c3c9b416 100644 --- a/substrate/substrate/runtime/council/src/lib.rs +++ b/substrate/substrate/runtime/council/src/lib.rs @@ -549,7 +549,7 @@ impl Module { #[serde(rename_all = "camelCase")] #[serde(deny_unknown_fields)] pub struct GenesisConfig { - // for the voting onto the council + // for the voting onto the council pub candidacy_bond: T::Balance, pub voter_bond: T::Balance, pub present_slash_per_voter: T::Balance, diff --git a/substrate/substrate/runtime/executive/src/lib.rs b/substrate/substrate/runtime/executive/src/lib.rs index 52cfda24c9..8551580df7 100644 --- a/substrate/substrate/runtime/executive/src/lib.rs +++ b/substrate/substrate/runtime/executive/src/lib.rs @@ -82,14 +82,15 @@ pub struct Executive< >(PhantomData<(System, Block, Lookup, Payment, Finalisation)>); impl< + Address, System: system::Trait, Block: traits::Block, - Lookup: AuxLookup::Address, Target=System::AccountId>, + Lookup: AuxLookup, Payment: MakePayment, Finalisation: Executable, > Executive where - Block::Extrinsic: Checkable + Slicable, - ::Checked: Applyable + Block::Extrinsic: Checkable Result> + Slicable, + Result>>::Checked: Applyable { /// Start the execution of a particular block. pub fn initialise_block(header: &System::Header) { @@ -172,7 +173,7 @@ impl< /// Actually apply an extrinsic given its `encoded_len`; this doesn't note its hash. fn apply_extrinsic_no_note_with_len(uxt: Block::Extrinsic, encoded_len: usize) -> result::Result { // Verify the signature is good. - let xt = uxt.check(Lookup::lookup).map_err(internal::ApplyError::BadSignature)?; + let xt = uxt.check_with(Lookup::lookup).map_err(internal::ApplyError::BadSignature)?; if xt.sender() != &Default::default() { // check index diff --git a/substrate/substrate/runtime/primitives/src/generic.rs b/substrate/substrate/runtime/primitives/src/generic.rs index 06c7000372..863dfb7735 100644 --- a/substrate/substrate/runtime/primitives/src/generic.rs +++ b/substrate/substrate/runtime/primitives/src/generic.rs @@ -96,7 +96,7 @@ impl UncheckedExtrinsic traits::Checkable +impl traits::Checkable for UncheckedExtrinsic> where Address: Member + Default + MaybeDisplay, @@ -106,18 +106,11 @@ where AccountId: Member + Default + MaybeDisplay, ::MaybeUnsigned: Member, Extrinsic: Slicable, + ThisLookup: FnOnce(Address) -> Result, { - type Address = Address; - type AccountId = AccountId; type Checked = CheckedExtrinsic; - fn sender(&self) -> &Address { - &self.extrinsic.signed - } - - fn check(self, lookup: ThisLookup) -> Result where - ThisLookup: FnOnce(Address) -> Result, - { + fn check_with(self, lookup: ThisLookup) -> Result { if !self.is_signed() { Ok(CheckedExtrinsic(Extrinsic { signed: Default::default(), diff --git a/substrate/substrate/runtime/primitives/src/testing.rs b/substrate/substrate/runtime/primitives/src/testing.rs index 7c712b4010..12e347e1e3 100644 --- a/substrate/substrate/runtime/primitives/src/testing.rs +++ b/substrate/substrate/runtime/primitives/src/testing.rs @@ -157,12 +157,9 @@ impl Checkable for TestXt { +impl Checkable for TestXt { type Checked = Self; - type Address = u64; - type AccountId = u64; - fn sender(&self) -> &u64 { &(self.0).0 } - fn check Result>(self, _lookup: ThisLookup) -> Result { Ok(self) } + fn check_with(self, _: Context) -> Result { Ok(self) } } impl + Slicable + Sized + Send + Sync + Serialize + DeserializeOwned + Clone + Eq + Debug> Applyable for TestXt { type AccountId = u64; diff --git a/substrate/substrate/runtime/primitives/src/traits.rs b/substrate/substrate/runtime/primitives/src/traits.rs index 9f618e4dbc..77eaba3fd2 100644 --- a/substrate/substrate/runtime/primitives/src/traits.rs +++ b/substrate/substrate/runtime/primitives/src/traits.rs @@ -375,31 +375,32 @@ pub type HashFor = <::Header as Header>::Hashing; /// A "checkable" piece of information, used by the standard Substrate Executive in order to /// check the validity of a piece of extrinsic information, usually by verifying the signature. -pub trait Checkable: Sized + Send + Sync { - type Address: Member + MaybeDisplay; - type AccountId: Member + MaybeDisplay; - type Checked: Member; - fn sender(&self) -> &Self::Address; - fn check Result>(self, lookup: ThisLookup) -> Result; +/// Implement for pieces of information that require some additional context `Context` in order to be +/// checked. +pub trait Checkable: Sized { + /// Returned if `check_with` succeeds. + type Checked; + + fn check_with(self, context: Context) -> Result; } /// A "checkable" piece of information, used by the standard Substrate Executive in order to /// check the validity of a piece of extrinsic information, usually by verifying the signature. -/// -/// This does that checking without requiring a lookup argument. -pub trait BlindCheckable: Sized + Send + Sync { - type Address: Member + MaybeDisplay; - type Checked: Member; - fn sender(&self) -> &Self::Address; +/// Implement for pieces of information that don't require additional context in order to be +/// checked. +pub trait BlindCheckable: Sized { + /// Returned if `check` succeeds. + type Checked; + fn check(self) -> Result; } -impl Checkable for T { - type Address = ::Address; - type AccountId = ::Address; +// Every `BlindCheckable` is also a `Checkable` for arbitrary `Context`. +impl Checkable for T { type Checked = ::Checked; - fn sender(&self) -> &Self::Address { BlindCheckable::sender(self) } - fn check Result>(self, _: ThisLookup) -> Result { BlindCheckable::check(self) } + fn check_with(self, _: Context) -> Result { + BlindCheckable::check(self) + } } /// An "executable" piece of information, used by the standard Substrate Executive in order to diff --git a/substrate/substrate/test-runtime/src/lib.rs b/substrate/substrate/test-runtime/src/lib.rs index 1b07035e96..ee4357aaee 100644 --- a/substrate/substrate/test-runtime/src/lib.rs +++ b/substrate/substrate/test-runtime/src/lib.rs @@ -119,11 +119,7 @@ impl Slicable for Extrinsic { impl BlindCheckable for Extrinsic { type Checked = Self; - type Address = AccountId; - fn sender(&self) -> &Self::Address { - &self.transfer.from - } fn check(self) -> Result { if ::runtime_primitives::verify_encoded_lazy(&self.signature, &self.transfer, &self.transfer.from) { Ok(self)