feat: add experimental XCM CE with companion pallet #152

ashutoshvarma · 2023-05-04T06:11:16Z

Pull Request Summary
Add the experimental XCM CE along with companion pallet.

Callback Design

The callback design make use of pallet_xcm's OnResponse handler which has capability to notify a dispatch on a XCM response (if notify query is registered).

For us that dispatch is companion pallet's on_callback_received which will route the xcm response (Response enum) back to contract via a bare_call (if wasm contract)

Let's understand the control flow,

Contract A will call the XCM CE (XCMExtension) to register a new query of type QueryType::WasmContractCallback { .. } .
CE/Precompile’s will in-turn call XCM Transact’s new_query
Register the query with help of pallet_xcm's new_notify_query and sets the notify’s dispatch to its dispatchable on_callback_recieved() and saves the QueryType for the registered query_id and return the query_id.
Contract A send the XCM with help of CE/Precompile
CE/Precompile forward the XCM to XCM Transact
XCM Transact forward the XCM to Pallet Xcm
which finally sends it to destination
Destination sends the response back which is intercepted by pallet_xcm ’s OnResponse
pallet_xcm’s OnResponse calls the on_callback_recieved()
on_callback_recieved() on receiving response calls the CallbackHandler
CallbackHandler will then route the response based on the QueryType , for example to a Wasm contract’s method

Open Questions

1. Who pays for the execution of callback?

In current design, when registering a query callback weight is not taken into account. This is because the weight of callback/notify is set inside the query instruction itself and query method does not take xcm as input (chicken and egg problem), for example in the below instruction.

ReportTransactStatus(QueryResponseInfo {
    destination: (Parent, Parachain(1)).into(),
    query_id,
    max_weight: Weight::from_parts(100_000_000_000_000, 1024 * 1024 * 1024)),    -----> this will be copied to `QueryResponse` that will be sent to querier chain
})

For the above query instruction below query response would be sent and max_weight is copied to it. The OnResponse handler will ensure that notify dispatch's weight is less than max_weight before calling it.

QueryResponse {
	query_id,
	response,
	max_weight: Weight::from_parts(100_000_000_000_000, 1024 * 1024 * 1024)),
	querier: Some(Here),
},

Therefore, the weight for callback and for processing the callback should be charged when when sending the XCM.
This can be achieved in two manners,

via WeightInfoBounds: where all query instructions's QueryResponseInfo::max_weight is taken into account. Since weight of pallet_xcm::send() dispatch also includes the XCM weight.
Kusama uses WeightInfoBounds but does not take max_weight into account, it would be similar but also take max_weight into account.
via PriceForSiblingDelivery - When calculating the XCM sending fee, this is ideal place for it actually. The XCM sending fee should take into account the max_weight from every query instruction. Right now, PriceForSiblingDelivery in xcm router is set to ().

3. What happens when callback failed?

If in any situation callback fails due to any reason (gas limit, wasm trapped, etc) that would quite difficult for contract to manage. It's still not fatal because contract can implement it's own query timeout based on block number and consider the query expired in such case. Having said that, it would be a very good idea to save the response for contract to manually poll later as a fallback in case of callback error.
Obviously rent fee + weight associated with it should be charged during query registration itself which will make registering query a bit costlier.
Still a TODO

4. How is rent fee handled for registering query and rent refund?

Not handled right now, still a TODO

5. Query Ids cannot grow indefinitely

We are using pallet_xcm's query mechanism for this registering/managing queries which does not recycle query id, that means query id is incremental, thus limited. On top of that, pallet_xcm uses u64 for query ids, which although has very large max value but still can be a issue in future (well if that comes, we'll probably need a storage migration to change it to u128).

TODO

move test scenarios to this PR and add more (need xcm simulator?)
figure out overall weights (new query, take response, etc) via benchmarks
figure out rent price and refund
save response on callback failure

Check list

added unit tests
updated documentation

2708b052 wip: move error to CE from pallet 8441e0f3 wip: add event for callback failure 841a6ae6 wip: remove `ink_env` dep from primitives 24b61a18 wip: change xcm ce extension id to 04 f2a20149 wip: minor refactoring and add to shibuya 4ee1d597 wip: lot of refactoring for ce types eb8af147 wip: rename types to primitives, add new query disptach 8ce0b068 wip: complete CE + basic tests 20612bd5 wip: add pallet+CE

Dinonard

When you need a review for a large commit, you should prepare better.

The first thing someone sees is the PR summary which is in this case 1 sentence.
That's fine if you're adding a few changes and it's obvious from the code.
For your case, with 1200 LoC, it's not enough.

Please expand it and give a summary of what you're doing, the structure, assumptions/simplifications, etc. The less effort other person has to spend on understanding
what you're trying to say, the more you'll get from the interaction.

A direct follow-up would be the rustdoc of the lib.rs files which is non-existant, essentially.
Personally, when I open a new piece of code (e.g. a pallet), I will first check the rustdoc at the top. 🤷‍♂️

My recommendation is to polish the PR summary, docs and code (maybe cleanup some unnecessary TODOs?).

For the functionality wise, it's an interesting solution!
Clearly significant amount of effort was put into this, well done! 💪

Maybe I missed this but it's unclear to me:

who pays for the execution of receiving the query response? (not the smart contract call, just the query processing)
rent fee isn't handled
query Ids cannot grow indefinitely (I know you're aware of this)

Such questions/explanations should be in PR summary, IMO.

frame/pallet-xcm-transactor/src/lib.rs

Dinonard · 2023-05-05T13:52:12Z

frame/pallet-xcm-transactor/src/lib.rs

+        /// Register a new query
+        /// THIS IS ONLY FOR WEIGHTS BENCHMARKING, since we cannot benchmark non-dispatch


You can benchmark anything actually, just call that function in the benchmark macro. These calls don't have to be extrinsic calls.

I didn't know that awesome.
I'll try benchmarking the non dispatch new_query and remove this afterwards

Dinonard · 2023-05-05T14:02:34Z

frame/pallet-xcm-transactor/src/lib.rs

+    pub struct Pallet<T>(_);
+
+    #[pallet::config]
+    pub trait Config: frame_system::Config + pallet_xcm::Config + pallet_contracts::Config {


That is tight coupling with pallet_contracts. Fine for PoC, but for production, it's better to handle such requirements via associated types.

Dinonard · 2023-05-05T14:08:51Z

frame/pallet-xcm-transactor/src/chain_extension.rs

+    fn prepare_execute<E: Ext<T = T>>(
+        &mut self,
+        env: Environment<E, InitState>,
+    ) -> DispatchResult<RetVal> {
+        let mut env = env.buf_in_buf_out();
+        // input parsing
+        let len = env.in_len();
+        let input: VersionedXcm<RuntimeCallOf<T>> = env.read_as_unbounded(len)?;
+
+        let mut xcm = unwrap!(input.try_into(), BadVersion);
+        // calculate the weight
+        let weight = unwrap!(T::Weigher::weight(&mut xcm), CannotWeigh);
+
+        // save the prepared xcm
+        self.prepared_execute = Some(PreparedExecution { xcm, weight });
+        // write the output to buffer
+        weight.using_encoded(|w| env.write(w, true, None))?;
+
+        Ok(RetVal::Converging(XcmCeError::Success.into()))
+    }


What's the weight associated with this?

Can it be spammed until resources are exhausted?

Nothing as of now, weights are overall a TODO still.
Best choice would be moving most of this logic to pallet and benchmark that for weights.

Dinonard · 2023-05-05T14:12:17Z

frame/pallet-xcm-transactor/src/chain_extension.rs

+    /// Register the new query
+    /// TODO: figure out weights


Weight of registration, weight of execution (and payment for it), rent fee (and reimbursment?).

Only weight of registration and rent fee (still a TODO).
Execution weight will be paid while sending the XCM calculated via PriceForSiblingDelivery in cumulus_pallet_xcmp_queue config which right now is set to ().

This is because the weight of callback/notify is set inside the query instruction itself and query method does not take xcm as input (chicken and egg problem), for example in the below instruction.

ReportTransactStatus(QueryResponseInfo { destination: (Parent, Parachain(1)).into(), query_id, max_weight: Weight::from_parts(100_000_000_000_000, 1024 * 1024 * 1024)), -----> this will be copied to `QueryResponse` that will be sent to querier chain })

For the above query instruction below query response would be sent and max_weight is copied to it. The OnResponse handler will ensure that notify dispatch's weight is less than max_weight before calling it.

QueryResponse { query_id, response, max_weight: Weight::from_parts(100_000_000_000_000, 1024 * 1024 * 1024)), querier: Some(Here), },

EDIT: or if I remember correctly, pallet_xcm::send dispatch also takes care of XCM weight into its weight? In that case WeightInfoBounds should take care of max_weight from every query instruction

Ok, I'm not sure that PriceForSiblingDelivery is supposed to take care of that.
In the context we're talking about, it's charging the processing of query response (even before it's polled from the smart contract).

I still don't think that current pallet-xcm system will be sufficient to handle this scenario properly 🙂 .
BUT - I'd have to spend some time on investigating this to really be sure and make concrete suggestions.

pgherveou · 2023-06-07T08:34:22Z

frame/pallet-xcm-transactor/src/chain_extension/mod.rs

+impl<T: Config, W: CEWeightInfo> XCMExtension<T, W> {
+    /// Returns the weight for given XCM and saves it (in CE, per-call scratch buffer) for
+    /// execution
+    fn prepare_execute<E: Ext<T = T>>(


can you explain why we need prepare_execute and why it can not be done as a first step when invoking execute ?

No particular reason, It can be done in one step.

I started this by updating the original XCM CE PoC (by Alex) for XCMv3 and it had this structure so I just kept it.
Also it is kind of useful for smart contract developers to get the execution weight before actually executing it.

ashutoshvarma added 4 commits May 4, 2023 09:55

docs: add license header

9e2b137

feat: check querier in take response

2617963

feat: move xcm ce simulator tests to astar-frame

8a5054f

Dinonard reviewed May 5, 2023

View reviewed changes

ashutoshvarma added 11 commits May 9, 2023 07:55

wip(refactor): move ink sdk into seperate crate

4d29c05

wip: cleanup simulator tests and add fixtures

cf44fd9

wip: add cargo build script to compile contracts for fixtures

c275839

wip: add readme and fix some dcos

c21cc8a

wip: add docstring for pallet

342a893

wip: move most logic to pallet for bechmarking

1b34116

wip: add benchmarks and weights

0826653

wip: add weights in CE calls

46ec701

wip: refactor pallet

889dad5

wip: update simulator tests

b172aa6

wip: fix benchamrks features build

75cc056

pgherveou reviewed Jun 7, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add experimental XCM CE with companion pallet #152

feat: add experimental XCM CE with companion pallet #152

ashutoshvarma commented May 4, 2023 •

edited

Dinonard left a comment

Dinonard May 5, 2023

ashutoshvarma May 9, 2023

Dinonard May 5, 2023

Dinonard May 5, 2023

ashutoshvarma May 9, 2023

Dinonard May 5, 2023

ashutoshvarma May 9, 2023 •

edited

Dinonard May 9, 2023

pgherveou Jun 7, 2023

ashutoshvarma Jun 7, 2023

		/// Register a new query
		/// THIS IS ONLY FOR WEIGHTS BENCHMARKING, since we cannot benchmark non-dispatch

feat: add experimental XCM CE with companion pallet #152

Are you sure you want to change the base?

feat: add experimental XCM CE with companion pallet #152

Conversation

ashutoshvarma commented May 4, 2023 • edited

Callback Design

Open Questions

1. Who pays for the execution of callback?

3. What happens when callback failed?

4. How is rent fee handled for registering query and rent refund?

5. Query Ids cannot grow indefinitely

Dinonard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashutoshvarma May 9, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashutoshvarma commented May 4, 2023 •

edited

ashutoshvarma May 9, 2023 •

edited