BaseParams for SignedExtensions to allow for client-derived defaults in CheckMortality and CheckNonce

[tadeohepperle]

Fixes #202

What problem does this solve?

This PR aims at streamlining the process of setting signed extensions. Signed extensions implement the ExtrinsicParams trait. This trait has an associated type Params. (renamed from OtherParams).
Signed extensions are added to an extrinsic in roughly 4 steps:

1. Get some data from the client: e.g. current account nonce, metadata, genesis hash, runtime version, ...
   This data is stored in a struct called BaseParams.
2. Use the BaseParams from (1) to create Params. Params need to implement FromBaseParams, which just allows us to create default values fro Params in the presense of BaseParams.
3. Use BaseParams from (1) and Params (e.g. CheckMortalityParams) from (2) to create the signed extension (e.g. CheckMortality). This is done in the ExtrinsicParams::new() function.
4. Use the created signed extension(s) from (3) to encode extra and additional bytes to two buffers that are part of the extrinsic payload. This is done in the functions of the ExtrinsicParamsEncoder trait that all ExtrinsicParams must implement.

This process allows for creating extrinsics by default from BaseParams and nothing else, because everything else can be derived from there. But it also allows for skipping step (2) and instead providing Params defined by the user as a step (0).

This allows the CheckMortality signed extension to take a look at the current block hash and number and then construct a transaction that is valid for 32 blocks as suggested by @JesseAbram

It also allows account nonces to be automatically derived from BaseParams or defined manually.

[jsdw]

This is a clever approach, and is probably the most minimally breaking way to solve the problem!

One hesitation I have is that the additional trait adds a bit of complexity to things. Another general thing I don't like in this whole area of creating extrinsics is that we have functions like:

pub fn create_signed_with_nonce<Call, Signer>(
    &self,
    call: &Call,
    signer: &Signer,
    account_nonce: u64,
    other_params: <T::ExtrinsicParams as ExtrinsicParams<T>>::OtherParams,
) -> Result<SubmittableExtrinsic<T, C>, Error>

Which take in other_params and also the account_nonce, even though the latter is really just another param to signed extensions just like the rest.. it's just one we can't have a sensible default for. But that's what you've just addressed here!

So here's my possibly crazy idea:

I wonder what things would look like if we went all in on this idea of having a new trait to pass additional params to signed extensions, and used it for account_nonce too so that we could get rid of the *_with_nonce functions entirely and all params come from the same place?

So eg I wonder if something like this would work:

• Add account_nonce to DefaultExtrinsicParamsBuilder (probably as an argument to new so that it's always given).
• Expect it to then exist as an OtherParam to CheckNonce, so it can be configured from that instead
• Remove account_nonce from the ExtrinsicParams trait, so it only takes in Client and OthherParams.
• Instead of DefaultOrFrom which accepts Option<T>, do one of the following:

a) Change the DefaultOrFrom trait to expect both block hash and nonce like the following:

trait DefaultOtherParams<T: Config> {
    fn default_other_params(nonce: u64, latest_finalized_hash: T::Hash) -> Self
}

(I don't think we need to ever accept Option<T::Hash> here since when we call this we always do it with a hash)

Or

b) Could we just use From now? eg

struct MandatoryParams<T: Config> {
    pub current_finalized_hash: T::Hash,
    pub account_nonce: u64
}

impl <T: Config> From<MandatoryParams<T>> for () {
    fn from(params: MandatoryParams<T>) -> () {
        ()
    }
}

// ... impl on tuples of different sizes and expect to be implemented for `OtherParams` to allow us to construct them.

Either way basically means that a user can construct OtherParams as before, but then internally we can construct them too, providing the correct nonce and latest finalized hash whenever we do.

Finally, I'd:

• Put that trait in a new file with impls like what you've done to cover tuples for ease of use.
• Probably I'd now rename OtherParams to something else (they arent the "other" params now really, they are all of the params for extrinsics... maybe just Params?)

What do you reckon?

[jsdw]

My original hope here was that we could only ask for params from the user, and one could then construct params by providing an account nonce. It turns out that this complicates things a bit though; I guess I'd assumed that if a nonce wasn't provided in params then subxt would gather and inject one (which could be the same for mortality too, ie if not explicitly configured in params then we always default it to mortal, rather than just accept that it's immortal.

Having to ask for base_params and params doesn't really make anything nicer API wise in the end and doesn't allow us to remove any with_nonce methods, just rename them to something less nice (and I don't see why the user should ever want/need to provide genesis hash etc). Also, if I'd known we'd have a BaseParams I'd probably have kept OtherParams. We also have this whole BaseClient thing now which adds some complexity, so I'm left wondering if this is really what we want.

What it all makes me wonder is if there exists an elegant way to:

• Allow the user to provide OtherParams as before, and not needing to configure any explicit mortality or nonce.
• If no mortality is configured, default to mortal TX for 32 blocks.
• If no account nonce is configured then default to the current user nonce as before.
• OtherParams has a sensible default as before so that it doesn't need to be provided at all in the comon case where the defaults will suffice.

I realise that the original trait which constructs OtherParams from some set of default values (eg current block hash) probably is no good here, because to do the above, we want to enrich some existing params rather than just create them if not provided.

Potentially we could have a trait like this (naming tbc):

traut EnrichParams<T: Config> {
    // default impl just does nothing, so is easy to impl for signed exts that don't care
    fn enrich_params(&mut self, account_nonce: u64, finalized_hash: T::Hash) {}
}

Then we'd do something like:

• Add impl for CheckNonce and CheckMortality such that if no nonce/mortality configured, we configure them. these signed ext params might then involve an Option or whatever internally to denote that they should take this configuration.
• Add impls for tuples so that we can run it on a tuple of params to have it enrich each of them, updating as needed.
• If no enriching occurs, we do need to fall back to some sane defaults (eg when a tx is created entirely offline) when using the params to build the signed exts or whatever (this is a shame, but basically means one must provide account nonce and mortality else it will have to be immortal with nonce of 0. This is the bit I'm least happy about here.
• Do enriching in the appropriate place(s) in online tx client which then configures the mortality and nonce appropriately if no explicit values given for them already.

Then, we wouldn't need any concept of BaseClient or BasePartams, and whether you pass params or not you'll get a tx with default mortality and nonce configured if you don't otherwise provide them (rather than the default mortality always being immortal if you pass params explicitly).

What do you think?

[tadeohepperle]

Thinking about it again, I have to say that I agree, it is not too much of an improvement.
The idea of having this EnrichParams trait is interesting, it reminds me a bit of a trait called Refinable that I saw, when reading through the source code of the zed code editor, here. I like this idea.
For the nonce, we would have a CheckNonceParams(Option<u64>) that can be set by the user and implements EnrichParams, right?. We can then always run enrich_params on this: if the option is already Some(_), we leave it be, otherwise we set it to Some(account_nonce). Do I understand this correctly? Then the CheckNonce(u64) Signedextension can be built by just providing CheckNonceParams(Option<u64>) and nothing else. Is that in the spirit of the idea? So the flow would be:

• (1) create OtherParams manually or with Default::default()
• (2) refine them by OtherParams::enrich_params(..).
• (3) create the SignedExtension from OtherParams *
  Then we essentially move some of the compile time checks to the runtime by unwrapping the option. Or maybe I understand this incorrectly.

* Also where would be get access to the runtime_version for e.g. the CheckSpecVersion signed extension? Would we still need to pass a client in when the SignedExtension is create in step (3)?

The reason I implemented the BaseParams thingy is, that I wanted to reduce the number of arguments in the trait, while also getting rid of the generic client parameter in the new function. For example a signature like this: enrich_params(&mut self, account_nonce: u64, finalized_hash: T::Hash) is probably not enough. We also need to block number (or just the entire header) to construct a mortal CheckMortality signed extension.
We also need the runtime_version at some point when creating the signed extension for the CheckSpecVersion signed extension. I wanted to bundle all of this up in a simple BaseParams struct that provides a context that contains everything needed.

[tadeohepperle]

I am closing this PR in favor of #1439, created a new branch to try out the approach suggested by @jsdw