Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Add HrmpChannelManagementHooks #2661

Closed
wants to merge 14 commits into from
Closed

Conversation

4meta5
Copy link
Contributor

@4meta5 4meta5 commented Mar 22, 2021

As of now, to open, accept, and close channels on the relay chain, parachains must send the message Xcm::Transact with the formatted call data for hrmp dispatchables. Forming this call is hard from the parachain runtime; this Acala demo uses polkadot-js to form the call from the relay chain context and remove the prefix.

There is a xcm-format PR which includes three new Xcm variants to open, accept, and close channels. This PR adds those variants and the execution logic. This will make it unnecessary to use Xcm::Transact for those actions.

use xcm::v0::{Result as XcmResult};
impl<T: Config> ExecuteHrmp for Module<T> {
fn hrmp_init_open_channel(sender: u32, recipient: u32, max_message_size: u32, max_capacity: u32) -> XcmResult {
Self::init_open_channel(sender.into(), recipient.into(), max_capacity, max_message_size).map_err(|_| ().into())
Copy link
Contributor Author

@4meta5 4meta5 Mar 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now, this is returning the XcmError::Undefined if a runtime error is returned. What error should it be?

runtime/rococo/src/lib.rs Outdated Show resolved Hide resolved
@bkchr bkchr requested a review from pepyakin March 25, 2021 09:47
@4meta5
Copy link
Contributor Author

4meta5 commented Mar 25, 2021

This PR is ready for review. The CI is red because there are no labels (and I cannot set this). Let me know of suggestions @pepyakin @shawntabrizi

@4meta5 4meta5 mentioned this pull request Mar 25, 2021
1 task
@4meta5
Copy link
Contributor Author

4meta5 commented Mar 26, 2021

I received feedback that the Xcm message variants included in the referenced spec PR are not finalized so this PR will not be reviewed until that spec PR is merged.

I assumed the messages included in this PR were uncontroversial because they correspond directly to the actions for interacting with the hrmp pallet. This other spec PR polkadot-fellows/xcm-format#18 was approved and it includes the change with the same intention of providing a proper way to dispatch HRMP actions from the parachain. The only way to do this now is to compute the encoded calls in polkadot-js from the relay chain context, remove the prefix, and pass that to whatever code is forming the Xcm::Transact message.

@cla-bot-2021
Copy link

cla-bot-2021 bot commented Jun 3, 2021

User @4meta5, please sign the CLA here.

@apopiak apopiak added A0-please_review Pull request needs code review. B1-releasenotes C1-low PR touches the given topic and has a low impact on builders. D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited. labels Jul 27, 2021
xcm/src/v0/traits.rs Outdated Show resolved Hide resolved
xcm/src/v0/traits.rs Outdated Show resolved Hide resolved
HrmpCloseChannel {
#[codec(compact)] sender: u32,
#[codec(compact)] recipient: u32,
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recalling an open channel request is upcoming in #3543

I think it's worth to reconsider these messages in the light of that PR.

@@ -29,6 +29,9 @@ pub trait Config {
/// How to withdraw and deposit an asset.
type AssetTransactor: TransactAsset;

/// How to execute HRMP-related actions
type HrmpExecutor: ExecuteHrmp;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this could be improved by specifying what exactly actions are (i.e. the channel management). It is also better to take into account how the developers will approach it. I think this is mostly relevant for relay-chains which is a way less common case. Thus it would be good to guide a typical implementer of this to use the () dummy

Co-authored-by: Sergei Shulepov <[email protected]>
pub trait ExecuteHrmp {
fn hrmp_init_open_channel(sender: u32, recipient: u32, max_message_size: u32, max_capacity: u32) -> Result;
fn hrmp_accept_open_channel(recipient: u32, sender: u32) -> Result;
fn hrmp_close_channel(initiator: u32, sender: u32, recipient: u32) -> Result;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DQ: why are all parameters u32 all across the place?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I presumed it's the same reason why we use u32 within XCM definitions. The alternative you are thinking of is ParaId. That comes from the primitives crate. Unlike one may think, it can be pretty heavy, bringing all kinds of cruft. At the same time, we want this crate to be used pervasively thus we want to minimize the number of dependencies.

xcm/xcm-executor/src/lib.rs Outdated Show resolved Hide resolved
@4meta5
Copy link
Contributor Author

4meta5 commented Aug 4, 2021

The new trait which exposes HRMP channel management functionality is:

pub trait HrmpChannelManagementHooks {
	type HrmpCall: Dispatchable + GetDispatchInfo;
	fn hrmp_init_open_channel(
		recipient: u32,
		max_message_size: u32,
		max_capacity: u32,
	) -> result::Result<Self::HrmpCall, Error>;
	fn hrmp_accept_open_channel(sender: u32) -> result::Result<Self::HrmpCall, Error>;
	fn hrmp_close_channel(sender: u32, recipient: u32) -> result::Result<Self::HrmpCall, Error>;
}

Each method returns a call so we can do the weight checks done for Xcm::Transact before dispatching the call. The other benefit of this approach is that the events will be emitted, before they were not emitted upon execution because we weren't dispatching the calls.

@4meta5 4meta5 changed the title Add HrmpExecutor Add HrmpChannelManagementHooks Aug 4, 2021
@4meta5
Copy link
Contributor Author

4meta5 commented Aug 4, 2021

Current error, PRing substrate with impl

error[E0277]: the trait bound `(): GetDispatchInfo` is not satisfied
   --> xcm/src/v0/traits.rs:282:2
    |
271 |     type HrmpCall: Dispatchable + GetDispatchInfo;
    |                                   --------------- required by this bound in `HrmpChannelManagementHooks::HrmpCall`
...
282 |     type HrmpCall = ();
    |     ^^^^^^^^^^^^^^^^^^^ the trait `GetDispatchInfo` is not implemented for `()`

@4meta5
Copy link
Contributor Author

4meta5 commented Aug 17, 2021

branch is stale, I don't have time now to update/maintain it

@4meta5 4meta5 closed this Aug 17, 2021
@shawntabrizi shawntabrizi removed the D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited. label Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants