Runtime: refactor Checkable trait to be more generic

[rphmeier]

paritytech/polkadot#195 (comment)

trait Checkable<T> {
    type Checked;

    fn check_with(&self, ctx: T) -> Result<Self::Checked, Self>;

    fn check(&self) -> Result<Self::Checked, Self> where Self: Checkable<(), Checked=Self::Checked> {
        self.check_with(())
    }
}

should be good enough for the trait definition.

[gitcoinbot]

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

---

This issue now has a funding of 0.35 ETH (164.11 USD @ $468.88/ETH) attached to it.

• If you would like to work on this issue you can 'start work' on the Gitcoin Issue Details page.
• Questions? Checkout Gitcoin Help or the Gitcoin Slack
• $26,563.50 more funded OSS Work available on the Gitcoin Issue Explorer

[snd]

should the BlindCheckable trait also get refactored similar to the Checkable trait or stay as is?

on second thought i think it makes sense to remove BlindCheckable and simply use Checkable<()> in its place.

using this for the moment:

pub trait Checkable<Ctx>: Sized {
    type Checked;

    fn check_with(self, ctx: Ctx) -> Result<Self::Checked, Self>;
}

/// can be checked without additional context.
pub trait BlindCheckable: Sized {
    type Checked;

    fn check(self) -> Result<Self::Checked, Self>;
}

[snd]

a clone/copy of self is required every time check returns an Err as well as when it returns Ok for the common case that Self::Checked == Self.

we could take check and check_with take ownership of self i.e. take self instead of &self to remedy this. this results in less/zero expensive clones for checks that create ::Checked from fields of self. settled on this for now

or alternatively we could change the return type to Result<Self::Checked, ()>.
there seems to be no need to give back Self since the caller already has it.

[snd]

with the above definition there's no way to communicate back why the check failed

[snd]

the above definition causes a cycle:

error[E0391]: cycle detected when computing the bounds for type parameter `Self`
   --> substrate/runtime/primitives/src/traits.rs:387:86 
    |                                                     
387 |     fn check(&self) -> Result<Self::Checked, Self> where Self: Checkable<(), Checked=Self::Checked> {

how about we simplify this to:

trait Checkable<Ctx> {
    type Checked;

    fn check_with(&self, ctx: Ctx) -> Result<Self::Checked, ()>;
}

to me this seems as expressive but simpler

[snd]

this https://github.com/paritytech/polkadot/blob/4f109124bbca456a99ea28614a9444c1c024653f/substrate/runtime/executive/src/lib.rs#L87 depends on the associated ::Address type of the "old" Checkable trait. @rphmeier how do you suggest we handle this now that the associated type no longer exists? maybe move ::Address to another suitable trait.

maybe moving ::Address to system::Trait would make sense? since AccountId which was also a member of Checkable is already there. moved ::Address to system::Trait for now

[rphmeier]

@snd I guess it's fine to only have check_with and we can extend with BlindCheckable. The goal is just that when checking requires no inputs that the call site be uncluttered by a ().

[snd]

working on this over here https://github.com/snd/polkadot/tree/snd-issue-212-checkable. still adapting all previous uses of the Checkable trait to the new.

@rphmeier the previous Checkable trait returned the error returned by the lookup function. the new proposed one doesn't. i now encountered a match over the returned error here: https://github.com/paritytech/polkadot/blob/ddc8d705570fda5f492895373b69b9b84cae1d69/polkadot/transaction-pool/src/lib.rs#L284 do we need to add returning some associated error type to the new Checkable trait. or could we simply change the linked code to:

Ok(xt) => Some(xt),
Err(_) => None,

?

not an option. that breaks it. going to add an associated ::Error type that is returned in case of errors.

related comment paritytech/polkadot#212 (comment)