Allow Dispatchable to return data

[athei]

We want to improve the user experience of calling into the runtime from a contract (#13641). While a contract can already call a Dispatchable only a subset of interactions can be crafted because a Dispatchable cannot return any data on success.

This is because a dispatchable is required to return a DispatchResult which is defined as Result<(), DispatchError>. In order to allow for a proper interaction which includes querying data from the runtime we need to allow a Dispatchable to return any type that implements Encode + TypeInfo.

I think it is viable to support this in backwards compatible matter without causing disruption: Whenever the Dispatchable is called by an extrinsic or from another place that doesn't need or want the output it is just discarded. This is not only useful for contracts but could also reduce the amount of runtime APIs that are basically getters: Instead of adding a new API to query data we could call a Dispatchable that returns data using system_dryRun. This potentially also allows RPC nodes to argue about the weight of operations they are performing for users.

Of course this change should be non breaking: Existing Dispatchables returning () should work as-is.

• Implement change that allows to return Result<T, DispatchError> where T: Encode + TypeInfo from a dispatchable.
• Check impact on emitted metadata.

[bkchr]

The problem is that people will expect that this also works for dispatchables included in a block. Then we expect people to send events and return values? Or we just add a lot of dummy runtime calls for returning values to contracts? I'm not convinced that this is a good way forward.

Your only problem with chain extensions is the benchmarking?

[xlc]

This is not only useful for contracts but could also reduce the amount of runtime APIs that are basically getters

This makes no sense to me. Runtime APIs are getters. Dispatchables are setters. Why do I want my setters to be able to replace getters?

[athei]

Your only problem with chain extensions is the benchmarking?

I noticed that many chain extensions are just forwarding to dispatchables or public pallet functions. The main problem is requiring users to write this glue code. Benchmarking is just one (but the biggest part) of it. Essentially I am looking for a way to completely get rid of glue code and just give users a way of defining getters within a pallet.

The glue code is completely dumb in this case. Users just want to interact with a pallets public interface. They can do that for dispatchables but not for functions returning data.

The problem is that people will expect that this also works for dispatchables included in a block. Then we expect people to send events and return values? Or we just add a lot of dummy runtime calls for returning values to contracts?

Yeah this is a problem. But to be fair those "dummy functions" for other runtime code to be consume already exists. Those are usually just public functions on a pallet. The way of making those accessible to users outside of the runtime is defining a runtime API that then calls this function.

I am wondering whether it would be better to define getters not as Runtime API but much more similar to a Dispatchable. Just another annotation to be used within a pallet that (optionally) contains a weight annotation. If no weight annotation is supplied than this function cannot be called from contracts. They would not be usable within an extrinsic but would only be used to query data.

This makes no sense to me. Runtime APIs are getters. Dispatchables are setters.

Dispatchables are not pure setters even right now. They just use a side channel for returning data (events).

Why do I want my setters to be able to replace getters?

Because runtime APIs don't have weight annotations. Hence they can't be called from contracts without adding annoying glue code. Unifying with dispatchables in one way or another would solve this problem.

Alternate Solutions

Let me list all the alternate approaches that come to mind that would allow a contract to read data from a pallet without having to add glue code:

1. Capture all the events emitted by an Dispatchable and copy them back to contracts memory when a contract calls a Dispatchable: This might be the easiest solution to implement. But parsing events is quite annoying. We want a slick solution where you can easily expose a function to be called by a contract.
2. Allow contracts to read arbitrary storage items. This seems like an elegant solution at first. However, runtime storage layout is not stable. A storage migration would not cover contracts depending on that layout. This is why we need stable getters.
3. Making the Dispatchable system more flexible. The suggested relaxation of the return value is only one idea. We could also think about defining different kinds of exposed functions (Dispatchable, Getter, CustomFunction). The Getter type of exposed function would be what I suggest above.
4. Double down on chain extensions. Removing the boiler plate by introducing macros that integrate with FRAME and generate the extension itself + metadata to be consumed by ink!. This would essentially recreate what FRAME is doing for the Call enum. Obvious downside is that it adds a lot of code to maintain that is basically doing the same that FRAME is doing. I would rather find a way to make the dispatchable system more flexible (3).

[bkchr]

I am wondering whether it would be better to define getters not as Runtime API but much more similar to a Dispatchable. Just another annotation to be used within a pallet that (optionally) contains a weight annotation. If no weight annotation is supplied than this function cannot be called from contracts. They would not be usable within an extrinsic but would only be used to query data.

Could go hand in hand with: paritytech/polkadot-sdk#216

2. However, runtime storage layout is not stable.

The same applies to the Call structure, this is also not "stable". The most stable solution is your current model, aka the ChainExtensions where you can do proper versioning etc.

[xlc]

or we can weight annotate runtime API, which is actually a requirement because they are all public by default on RPC nodes and could be a DoS vector #12698

[athei]

I am wondering whether it would be better to define getters not as Runtime API but much more similar to a Dispatchable. Just another annotation to be used within a pallet that (optionally) contains a weight annotation. If no weight annotation is supplied than this function cannot be called from contracts. They would not be usable within an extrinsic but would only be used to query data.

Could go hand in hand with: paritytech/polkadot-sdk#216

Yes. It is similar. But the way you proposed it in the linked issue it is more similar to a RuntimeAPI than a Call. It looks to me as a more comfortable way to define a subset of RuntimeAPIs. It is definitely good for replacing all those getter RuntimeAPIs.

I don't think we want to call those getters in a separate Wasm instance when calling from a contract. We are already inside the runtime. So we probably want to design it more like a Call. We construct a type we might call Getter which is basically like Call. We then implement a new trait Query (like Dispatchable) on it. All the getters are then called via a single Wasm exported function which accepts Getter. From within the runtime we can call it directly by using Query::query(). This will allow a contract to pass an encoded Query to the runtime.

Of course the getters would need weight annotations.

2. However, runtime storage layout is not stable.

The same applies to the Call structure, this is also not "stable". The most stable solution is your current model, aka the ChainExtensions where you can do proper versioning etc.

Yeah. Theoretically. But in practice we really rally try hard not to break it. Mostly for the fact that a Call could be scheduled with the Scheduler. ChainExtensions are pretty stable because they are manual work and it is a very conscious act to write them. As soon as we make it easy by providing nice macro to just expose pallet stuff as extensions the same oopsies can happen as with Call.

I think storage is in a different class. It is reorganized on purpose and it is not seen as a bad thing. We can't replace a changed or removed "storage" item with some compatibility function. It will just return wrong data. We can do that for a removed Call. There we could at least detect an error if the changed removed call was properly replaced by a compatibility function.

or we can weight annotate runtime API, which is actually a requirement because they are all public by default on RPC nodes and could be a DoS vector #12698

I think this alone wouldn't make it viable to be called from contracts. Reason is that RuntimeAPIs are meant to be called from outside the runtime. While a contract wants to call the code directly.

[bkchr]

So we probably want to design it more like a Call. We construct a type we might call Getter which is basically like Call. We then implement a new trait Query (like Dispatchable) on it. All the getters are then called via a single Wasm exported function which accepts Getter. From within the runtime we can call it directly by using Query::query(). This will allow a contract to pass an encoded Query to the runtime.

This sounds like a reasonable and good idea!

[athei]

Good :). Reposted into paritytech/polkadot-sdk#216. So we can probably close here then.