Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should we allow nested static calls to also benefit from a dynamic pallet/call index? #716

Closed
jsdw opened this issue Nov 11, 2022 · 4 comments

Comments

@jsdw
Copy link
Collaborator

jsdw commented Nov 11, 2022

Currently, if you create a static transaction, the pallet and call index that the call data will be encoded with are dynamic based on the node we're talking to.

But, if you create a nested call like so (see the multisig.rs example):

let inner_tx = polkadot::runtime_types::polkadot_runtime::RuntimeCall::Balances(
    polkadot::runtime_types::pallet_balances::pallet::Call::transfer {
        dest: dest.into(),
        value: 123_456_789_012_345,
    },
);

let tx = polkadot::tx().multisig().as_multi(
    // threashold
    1,
    // other signatories
    vec![signer_account_id],
    // maybe timepoint
    None,
    // call
    inner_tx,
    // max weight
    polkadot::runtime_types::sp_weights::weight_v2::Weight {
        ref_time: 10000000000,
        proof_size: 1,
    },
);

then tx itself will be encoded with a dynamic pallet/call index, but inner_tx will be encoded with a static index based on the codegen of polkadot::runtime_types::polkadot_runtime::RuntimeCall.

We have a trait called EncodeWithMetadata that basically takes a type ID, metadata and value and encodes to SCALE. Dynamic Value types implement it, and anything implementing Encode also implements it via a wrapper type.

Perhaps in the codegen, we could replace any reference to some_runtime::RuntimeCall in generated types with some type like StaticTxPayloadEncoded. This type might look something like:

pub struct StaticTxPayloadEncoded {
    pallet_name: String,
    call_name: String,
    call_data: Vec<u8>
};

impl EncodeWithMetadata for StaticTxPayloadEncoded {
    fn encode_with_metadata(&self, metadata, type_id) -> ... {
        // much like StaticTxPayload
    }
}

// any StaticTxPayload<T> can be partway-encoded into this type to remove the generic:
impl From<StaticTxPayload<T>> for StaticTxPayloadEncoded {

}

it might be that this StaticTxPayloadEncoded also implements TxPayload so that it can be directly passed into subxt as a valid transaction. Basically, it would allow the user to encode the same stuff that they would via RuntimeCall, but more easily.

If this new StaticTxPayloadEncoded arg was at most just a top level arg to some transaction calls, then we can manually implement EncodeWithMetadata on all of the call arg structs, call that via StaticTxPayload<T> when we need to encode and be done with it.

But some calls like utility.batch take args like Vec<RuntimeCall>, which we'd want to turn into Vec<StaticTxPayloadEncoded>. Ie, the call isn't the top level arg. To cater for this, we'd need to impl EncodeWithMetadata on every type that we generate (at least all those that could be used in call args, but easier just to blat it out for everything) so that it's recursively called on all nested things, including nested StaticTxPayloadEncoded's.

Perhaps it's worth splitting out EncodeWithMetadata into a separate crate with it's own derive macro so that it's trivial to implement instead of Encode on our generated types, and we can impl it on all core types as with Encode eg:

#[derive(EncodeWithMetadata)]
pub struct Foo {
    var: u32,
    wibble: SomethingElse
}

// impl would:
// - look at the type ID given, 
// - look up struct fields in the named composite type seen in metadata (or complain if we were expecting some non-struct-like type)
// - call encode_with_metadata() on all child types with the relevant type IDs handed down.

Long term, this would give all static types access to the relevant metadata and type info for what they are going to be encoded into. This lets things like StaticTxPayloadEncoded be "a little dynamic", and leaves the door open in case we need to add a sprinkling of dynamism elsewhere.

The downside is that we'd lose some performance over Encode and introduce all of this extra machinery to give us this dynamism.

The current alternative for users is to lean on the subxt::dynamic stuff for such cases, which, being fully dynamic in the way it encodes, will already handle this sort of case. But, it'd be nice to give people the chance to use our static codegen wherever possible, and this is currently a hole that makes static types worse for a use case they should be able to handle.

Any thoughts @ascjones?

@jsdw jsdw changed the title Allow nested static calls to also benefit from a dynamic pallet/call index Should we allow nested static calls to also benefit from a dynamic pallet/call index? Nov 11, 2022
@ascjones
Copy link
Contributor

Absolutely we should be able to use the static API for batching calls and similar. After all the static part is about the shape of the call data (MUST be compatible with target node) and the pallet/call indices have always been assumed to be "dynamic" because usually nodes are configured differently.

@jsdw
Copy link
Collaborator Author

jsdw commented Nov 11, 2022

Going further, if we double down on this idea of implementing EncodeWithMetadata on a load of types, one could imagine being able to submit transactions and such that were just a combination of static types like (123, "foo", vec![1,2,3]). Ie, some mix whereby you end up with the benefits of using static types (less allocating etc) but with the flexibility that the current dynamic Value stuff gives.

And yup, exactly; a static codegen interface leaning on this would then provide the correctness guarantee without losing out in terms of being too rigid and unable to cope with any variations in the metadata.

A derived impl of EncodeWithMetadata on enums for instance could always be resistant to index changes because it can line up the variant string against metadata each time. Derived impls on tuples and structs can align with metadata on name or position depending on what's available, and so on.

I feel like it could open the door to some nice API simplifications as the dynamic and static worlds get a bit closer while still retaining their core benefits.

@jsdw
Copy link
Collaborator Author

jsdw commented Nov 11, 2022

So to pull back what I said in chat, I think the approach to try this all out would be:

Step 1: make EncodeWithMetadata it's own crate with a derive macro and impls on all primitive types so that it's easy to construct deeply nested types that are static but know how to encode themselves using some runtime metadata.

Step 2: impl #[derive(EncodeWithMetadata)] on Subxt codegen types instead of Encode, do some basic substitution of RuntimeCall with some type like StaticTxPayloadEncoded so that it's easy to create nested calls just like outer ones, and generally just play with the APIs to see how best to fit these "slightly-less-static" and "fully dynamic" worlds together as best as possible (ie can we merge some stuff? What about storage queries etc).

@jsdw
Copy link
Collaborator Author

jsdw commented Apr 3, 2023

This can be closed now; the EncodeAsType/DecodeAsType stuff (see #842) means that now it will Just work. While I haven't added a nice wrapper around the generated RuntimeCall enum (I experimented a bit and didn't like the outcome offhand to far), the generated RuntimeCall enum will now automatically encode/decode based on variant names and not indexes :)

@jsdw jsdw closed this as completed Apr 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants