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

xcm version auto discovery improvements #922

Open
xlc opened this issue Oct 24, 2021 · 7 comments · May be fixed by #4025
Open

xcm version auto discovery improvements #922

xlc opened this issue Oct 24, 2021 · 7 comments · May be fixed by #4025
Assignees
Labels
T6-XCM This PR/Issue is related to XCM.

Comments

@xlc
Copy link
Contributor

xlc commented Oct 24, 2021

There are few issues with the current implement of xcm version auto discovery:

  • It doesn't work within transactional call. We revert all storage changes if the xcm message failed to sent, which reverts the update to VersionDiscoveryQueue.
  • It always fail for the first message by default, which is poor UX.
  • While there are governance calls to subscribe version, there is no permissionless way. It should allow user to trigger subscribe XCM version to all connected HRMP channel dest parachains.

Why not just automatically subscribe XCM version for all HRMP channel destination chains? Frontend can detect if the HRMP channel is created and if XCM version is discovered and pause feature until it is all good.

@gavofyork
Copy link
Member

HRMP happens at an abstraction level below XCM, so it would need to be done through an abstract interface. I’d consider a PR.

@xlc
Copy link
Contributor Author

xlc commented Oct 28, 2021

Accept HRMP channel already triggers a XCM message. We can automatically subscribe XCM version with HrmpChannelAccepted message?

https://github.com/paritytech/polkadot/blob/c50bdc8f0b36d47b38341b332d75e315cdd658f8/runtime/parachains/src/hrmp.rs#L1115

@KiChjang KiChjang added the T6-XCM This PR/Issue is related to XCM. label Dec 24, 2021
@Sophia-Gold Sophia-Gold transferred this issue from paritytech/polkadot Aug 24, 2023
claravanstaden pushed a commit to Snowfork/polkadot-sdk that referenced this issue Dec 8, 2023
@bkontur
Copy link
Contributor

bkontur commented Mar 14, 2024

Accept HRMP channel already triggers a XCM message. We can automatically subscribe XCM version with HrmpChannelAccepted message?

https://github.com/paritytech/polkadot/blob/c50bdc8f0b36d47b38341b332d75e315cdd658f8/runtime/parachains/src/hrmp.rs#L1115

@xlc It looks like I came to the same conclusion: #3692

@bkontur
Copy link
Contributor

bkontur commented Apr 8, 2024

Accept HRMP channel already triggers a XCM message. We can automatically subscribe XCM version with HrmpChannelAccepted message?

https://github.com/paritytech/polkadot/blob/c50bdc8f0b36d47b38341b332d75e315cdd658f8/runtime/parachains/src/hrmp.rs#L1115

@xlc proposed solution here: #4025 - initiate version discovery on HRMP channel accepted message.

If we want to immediately subscribe and set version with HrmpChannelAccepted, then we need to:

@bkontur
Copy link
Contributor

bkontur commented Apr 16, 2024

❌ The proposed solution #4025 won't work in the end because, as I understand it, the HRMP channel creation and closing are propagated to the parachain on the next relay chain session change. Therefore, this callback handler HrmpNotificationAccepted does not work as expected.

We need to find another solution for this because we don't have any callback when the HRMP channel is truly opened or closed on the parachain.

One possible solution would be to add some HrmpChannelHandler callback (on_create / on_close) to the parachain-system, perhaps somewhere in this section: https://github.com/paritytech/polkadot-sdk/blob/master/cumulus/pallets/parachain-system/src/lib.rs#L629:

<RelevantMessagingState<T>>::put(relevant_messaging_state.clone());

here, we could make a comparison between the actual <RelevantMessagingState<T>> stored on the parachain and the new relevant_messaging_state from the relay chain and to see ingress_channels and egress_channels changes.

Need to investigate more.

@xlc
Copy link
Contributor Author

xlc commented Apr 16, 2024

can we make the change so the notification is only sent when it is truly enacted? otherwise those notifications seems not very useful

@bkontur
Copy link
Contributor

bkontur commented Apr 16, 2024

can we make the change so the notification is only sent when it is truly enacted? otherwise those notifications seems not very useful

Yes, could be possible, but in that case, we would probably need a new HrmpNotifications, so xcm::v5, because there are some expectations for HrmpChannelAccepted / HrmpChannelClosing:

        /// A message to notify about that a previously sent open channel request has been accepted by
	/// the recipient. That means that the channel will be opened during the next relay-chain
	/// session change. This message is meant to be sent by the relay-chain to a para.
	///
	/// Safety: The message should originate directly from the relay-chain.
	/// 	
	HrmpChannelAccepted {..}


        /// A message to notify that the other party in an open channel decided to close it. In
	/// particular, `initiator` is going to close the channel opened from `sender` to the
	/// `recipient`. The close will be enacted at the next relay-chain session change. This message
	/// is meant to be sent by the relay-chain to a para.
	///
	/// Safety: The message should originate directly from the relay-chain.
	/// 
	HrmpChannelClosing {..}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T6-XCM This PR/Issue is related to XCM.
Projects
Status: In-Progress
5 participants
@gavofyork @xlc @KiChjang @bkontur and others