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 <UncheckedExtrinsic as Checkable>::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<Self::Checked, &'static str>` from `Checkable::check`

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()
 }

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 = <UncheckedExtrinsic as Checkable>::Checked;
+pub type CheckedExtrinsic = <UncheckedExtrinsic as Checkable<fn(Address) -> std::result::Result<AccountId, &'static str>>>::Checked;
 
 /// A verified transaction which should be includable and non-inherent.
 #[derive(Clone, Debug)]
@@ -281,7 +281,7 @@ impl<'a, A> txpool::Verifier<UncheckedExtrinsic> 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,

substrate/substrate/runtime/council/src/lib.rs

@@ -549,7 +549,7 @@ impl<T: Trait> Module<T> {
 #[serde(rename_all = "camelCase")]
 #[serde(deny_unknown_fields)]
 pub struct GenesisConfig<T: Trait> {
-	// 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,

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<Header=System::Header, Hash=System::Hash>,
-	Lookup: AuxLookup<Source=<Block::Extrinsic as Checkable>::Address, Target=System::AccountId>,
+	Lookup: AuxLookup<Source=Address, Target=System::AccountId>,
 	Payment: MakePayment<System::AccountId>,
 	Finalisation: Executable,
 > Executive<System, Block, Lookup, Payment, Finalisation> where
-	Block::Extrinsic: Checkable<AccountId=System::AccountId> + Slicable,
-	<Block::Extrinsic as Checkable>::Checked: Applyable<Index=System::Index, AccountId=System::AccountId>
+	Block::Extrinsic: Checkable<fn(Address) -> Result<System::AccountId, &'static str>> + Slicable,
+	<Block::Extrinsic as Checkable<fn(Address) -> Result<System::AccountId, &'static str>>>::Checked: Applyable<Index=System::Index, AccountId=System::AccountId>
 {
 	/// 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<internal::ApplyOutcome, internal::ApplyError> {
 		// 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

substrate/substrate/runtime/primitives/src/generic.rs

@@ -96,7 +96,7 @@ impl<Address, AccountId, Index, Call, Signature> UncheckedExtrinsic<Address, Ind
 	}
 }
 
-impl<Address, AccountId, Index, Call, Signature> traits::Checkable
+impl<Address, AccountId, Index, Call, Signature, ThisLookup> traits::Checkable<ThisLookup>
 	for UncheckedExtrinsic<Address, Index, Call, ::MaybeUnsigned<Signature>>
 where
 	Address: Member + Default + MaybeDisplay,
@@ -106,18 +106,11 @@ where
 	AccountId: Member + Default + MaybeDisplay,
 	::MaybeUnsigned<Signature>: Member,
 	Extrinsic<AccountId, Index, Call>: Slicable,
+	ThisLookup: FnOnce(Address) -> Result<AccountId, &'static str>,
 {
-	type Address = Address;
-	type AccountId = AccountId;
 	type Checked = CheckedExtrinsic<AccountId, Index, Call>;
 
-	fn sender(&self) -> &Address {
-		&self.extrinsic.signed
-	}
-
-	fn check<ThisLookup>(self, lookup: ThisLookup) -> Result<Self::Checked, &'static str> where
-		ThisLookup: FnOnce(Address) -> Result<AccountId, &'static str>,
-	{
+	fn check_with(self, lookup: ThisLookup) -> Result<Self::Checked, &'static str> {
 		if !self.is_signed() {
 			Ok(CheckedExtrinsic(Extrinsic {
 				signed: Default::default(),

substrate/substrate/runtime/primitives/src/testing.rs

@@ -157,12 +157,9 @@ impl<Call: AuxDispatchable + Slicable + Sized + Send + Sync + Serialize + Deseri
 		self.0.encode()
 	}
 }
-impl<Call: 'static + AuxDispatchable + Slicable + Sized + Send + Sync + Serialize + DeserializeOwned + Clone + Eq + Debug> Checkable for TestXt<Call> {
+impl<Call: Slicable + Sync + Send + Serialize + AuxDispatchable, Context> Checkable<Context> for TestXt<Call> {
 	type Checked = Self;
-	type Address = u64;
-	type AccountId = u64;
-	fn sender(&self) -> &u64 { &(self.0).0 }
-	fn check<ThisLookup: FnOnce(Self::Address) -> Result<Self::AccountId, &'static str>>(self, _lookup: ThisLookup) -> Result<Self::Checked, &'static str> { Ok(self) }
+	fn check_with(self, _: Context) -> Result<Self::Checked, &'static str> { Ok(self) }
 }
 impl<Call: AuxDispatchable<Aux = u64> + Slicable + Sized + Send + Sync + Serialize + DeserializeOwned + Clone + Eq + Debug> Applyable for TestXt<Call> {
 	type AccountId = u64;

substrate/substrate/runtime/primitives/src/traits.rs

@@ -375,31 +375,32 @@ pub type HashFor<B> = <<B as Block>::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<ThisLookup: FnOnce(Self::Address) -> Result<Self::AccountId, &'static str>>(self, lookup: ThisLookup) -> Result<Self::Checked, &'static str>;
+/// Implement for pieces of information that require some additional context `Context` in order to be
+/// checked.
+pub trait Checkable<Context>: Sized {
+	/// Returned if `check_with` succeeds.
+	type Checked;
+
+	fn check_with(self, context: Context) -> Result<Self::Checked, &'static str>;
 }
 
 /// 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<Self::Checked, &'static str>;
 }
 
-impl<T: BlindCheckable> Checkable for T {
-	type Address = <Self as BlindCheckable>::Address;
-	type AccountId = <Self as BlindCheckable>::Address;
+// Every `BlindCheckable` is also a `Checkable` for arbitrary `Context`.
+impl<T: BlindCheckable, Context> Checkable<Context> for T {
 	type Checked = <Self as BlindCheckable>::Checked;
-	fn sender(&self) -> &Self::Address { BlindCheckable::sender(self) }
-	fn check<ThisLookup: FnOnce(Self::Address) -> Result<Self::AccountId, &'static str>>(self, _: ThisLookup) -> Result<Self::Checked, &'static str> { BlindCheckable::check(self) }
+	fn check_with(self, _: Context) -> Result<Self::Checked, &'static str> {
+		BlindCheckable::check(self)
+	}
 }
 
 /// An "executable" piece of information, used by the standard Substrate Executive in order to

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<Self, &'static str> {
 		if ::runtime_primitives::verify_encoded_lazy(&self.signature, &self.transfer, &self.transfer.from) {
 			Ok(self)