runtime API: Substitute UncheckedExtrinsic with custom encoding

[lexnv]

The substrate::UncheckedExtrinsic type is substituted in codgen with a local type that implements custom encoding.

The old UncheckedExtrinsic(pub Vec<u8>) would re-encode the inner bytes of the extrinsic leading to a length prefix byte. This payload is then rejected by substrate WASM code, since the TransactionPaymentApi expects the bytes without the length prefix.

The following type was generated by the codegen:

pub struct UncheckedExtrinsic<_0, _1, _2, _3>(
    pub ::std::vec::Vec<::core::primitive::u8>,
    #[codec(skip)] pub ::core::marker::PhantomData<(_1, _0, _2, _3)>,
);

The type could not be passed to the runtime API because the encoding would expect only a sequence of data.
In contrast, this type is deduced to be a tuple.

Error: Encode(Error { context: Context { path: [Location { inner: Field("uxt") }] }, kind: WrongShape { actual: Tuple, expected: 13 } })

• Add a new type for UncheckedExtrinsic that is capable of passing through the provided bytes without any further encoding
• Substitute substrate::UncheckedExtrinsic with our custom UncheckedExtrinsic
• Validate a raw runtime API call with the statically generated API from codegen

Closes: paritytech/substrate#14584

[jsdw]

Potentially we could reuse some other code and avoid some duplicated logic by having this be:

pub struct UncheckedExtrinsic<Address, Call, Signature, Extra>{
    encoded: Static<Encoded>,
    #[codec(skip)]  _marker: PhantomData<(Address, Call, Signature, Extra)>,
};

Then, all of the impls could delegate to the Static<Encoded> type, which I think already does what we want it to.

In any case though, let's give this a nicer interface since users need to call it and shouldn't care about PhantomData etc, something like:

// make a new one:
UncheckedExtrinsic::new(bytes)
// can convert bytes into this type easily:
impl From<Vec<u8>> for UncheckedExtrinsic
// get the bytes:
unchecked_ext.bytes() -> &[u8]
// maybe convert back into bytes?:
impl From<UncheckedExtrinsic> for Vec<u8>

Hopefully type inference will sort out the generic params when it's used, ie

let runtime_api_call = node_runtime::apis()
        .transaction_payment_api()
        .query_fee_details(tx_bytes.into(), len);

Will hopefully Just Work, and infer all of the types properly!

[jsdw]

I like the test; great to check that this works :)

[jsdw]

Aah, I was hoping that you could get rid of more of this by using Static<Encoded> instead! This visitor trait is quite verbose alas.

I wonder whether something like this would work at least, to lean on the inner impl better (while not making any assumptions about it):

impl<Address, Call, Signature, Extra> Visitor
    for UncheckedExtrinsicDecodeAsTypeVisitor<Address, Call, Signature, Extra>
{
    type Value<'scale, 'info> = UncheckedExtrinsic<Address, Call, Signature, Extra>;
    type Error = scale_decode::Error;

    fn unchecked_decode_as_type<'scale, 'info>(
        self,
        input: &mut &'scale [u8],
        type_id: scale_decode::visitor::TypeId,
        types: &'info scale_info::PortableRegistry,
    ) -> DecodeAsTypeResult<Self, Result<Self::Value<'scale, 'info>, Self::Error>> {
        DecodeAsTypeResult::Decoded(self.0.decode_as_type(input, type_id.0, types))
    }
}

Maybe this is a sign that we can do something better with the DecodeAsType macro to allow things to be decoded based on some inner type or something.

[jsdw]

Ah alas we need to decalre it ourselves because the derive EncodeAsType impl will try to be smarter and make sure it's encoding this whole thing as a tuple, where we just want to defer to the Static impl and ignore this surrounding type entirely (this is why the test is failing)

[lexnv]

Yep exactly, we could at least remove the manual Encode part. I do wonder, could we make the EncodeAsType somehow ignore any fields that have #[codec(skip)] on them (similar to the codec)? 🤔

[jsdw]

It already automatically ignores any fields that the metadata isn't saying need to be encoded; the issue is more that it sees that it's on a tuple type and so expects the target type to be tuple-ish too (so that it knows how to encode the inner fields).

We'd want something like a #[transparent] flag or something to delegate to the inner impl :)

[jsdw]

And good idea on using Encode and Decode derives; that helps!

[lexnv]

Thanks that makes sense!

[tadeohepperle]

Looks good to me!

[tadeohepperle]

This formatting looks so weird, never seen that before :)

[lexnv]

Same, I used to write it without spaces but I've changed my editor to run rustfmt for each file save which changed it like so :D

[jsdw]

I think rustfmt is just being a bit naff here, but ah well :)

[jsdw]

Ah hmmm, I'm wondering about this automatic Decode, because:

• decoding like this will first decode a Compact length for the vec length and then put the relevant number of butes into the vec.
• encoding is not the opposite of this; it will just encode the bytes in the vec to the target; no compact length or whatever at the beginning.

Having a look, an extrinsic is actually just a compact encoded length and then that number of bytes.

So probably you were right in a previous iteration, and UncheckedExtrinsic should have a custom Decode impl that will decode the compact length and then the relevant number of bytes following it all into the Static<Encoded> struct, which will then encode (from the impl below) precisely those bytes back.

[jsdw]

All looks great; I think we just need to put back a manual impl of Decode on UncheckedExtrinsic which decodes the compact length and then following bytes and puts all of those bytes in a vec.

Conceptually something like:

fn decode(bytes) {
    // the exttrinsic is basically identical to the encoded form of this:
    let xt_vec = <Vec<u8>>:decode(&mut bytes)?;
    UncheckedExtrinsic(Static(Encoded(xt_vec.encode())))
}

We could save the extra vec allocation by manually decoding the compact length etc, but maybe it's easier just to do the above; we don't expect this to be a common op?

[jsdw]

Nice work!