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

Introduce Tinkernet multisig XCM configs to Kusama/Rococo through xcm-builder #7165

Merged
merged 14 commits into from Jun 28, 2023

Conversation

arrudagates
Copy link
Contributor

@arrudagates arrudagates commented May 2, 2023

This PR introduces the XCM configs required for Saturn multisigs from Tinkernet to transact on Kusama/Rococo, this is done simply by providing implementations to convert the MultiLocation to both an AccountId and a Signed RuntimeOrigin.

For more information regarding why this approach of using a very specific derivation function and defining this function in the receiver chain's runtime is necessary for Saturn, and also general information about Saturn, you can check the discussion that was opened in the forum prior: https://forum.polkadot.network/t/saturn-xcm-multisig-integration-on-kusama-a-technical-discussion/2694

For a more high level overview of Saturn, check this article: https://invarch.medium.com/saturn-the-future-of-multi-party-ownership-ac7190f86a7b

Please ask any questions left unanswered by the forum post and I'll make sure to clarify them!

Obs: My editor applied auto-format to some sections of the files edited, if those are unwanted for any reason, let me know and I'll revert them.
Those were actually the incorrect formatting standard required by CI, so I fixed them.

@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/saturn-xcm-multisig-integration-on-kusama-a-technical-discussion/2694/2

@arrudagates arrudagates changed the title Introduce Tinkernet multisig XCM configs Introduce Tinkernet multisig XCM configs to Kusama/Rococo May 2, 2023
@paritytech-ci paritytech-ci requested review from a team May 2, 2023 21:23
runtime/kusama/src/xcm_config.rs Outdated Show resolved Hide resolved
runtime/kusama/src/xcm_config.rs Outdated Show resolved Hide resolved
@arrudagates arrudagates requested review from xlc and rphmeier May 2, 2023 22:57
@ggwpez ggwpez requested review from KiChjang and xlc and removed request for xlc May 4, 2023 21:05
@arrudagates arrudagates requested a review from KiChjang May 5, 2023 15:49
xcm/xcm-builder/Cargo.toml Outdated Show resolved Hide resolved
@arrudagates arrudagates changed the title Introduce Tinkernet multisig XCM configs to Kusama/Rococo Introduce Tinkernet multisig XCM configs to Kusama/Rococo through xcm-builder May 5, 2023
@arrudagates arrudagates requested a review from KiChjang May 6, 2023 17:15
@arrudagates
Copy link
Contributor Author

@xlc @rphmeier Could you review this PR again?

@arrudagates
Copy link
Contributor Author

@KiChjang Could you add review requests to other maintainers in this PR?

@ggwpez ggwpez added D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. T1-runtime This PR/Issue is related to the topic “runtime”. B1-note_worthy Changes should be noted in the release notes labels Jun 27, 2023
@ggwpez ggwpez requested review from kianenigma and removed request for kianenigma June 27, 2023 22:09
@@ -345,6 +346,54 @@ impl<Network: Get<Option<NetworkId>>, AccountId: From<[u8; 20]> + Into<[u8; 20]>
}
}

/// Tinkernet ParaId used when matching Multisig MultiLocations.
pub const TINKERNET_PARA_ID: u32 = 2125;
Copy link
Contributor

Choose a reason for hiding this comment

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

This test is assuming your kusama para-id, right? it should be in the kusama runtime file, not in XCM-builder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed the constants to include the KUSAMA_ prefix.

@paritytech-ci paritytech-ci requested a review from a team June 28, 2023 08:39
@@ -241,6 +245,37 @@ where
}
}

/// Convert a Tinkernet Multisig `MultiLocation` value into a `Signed` origin.
pub struct TinkernetMultisigAsNative<RuntimeOrigin>(PhantomData<RuntimeOrigin>);
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly, I think from an organization perspective, it is more correct for this configuration to be placed in the kusama runtime, as it is the only user of it. But I see that it is more "convenient" to do it like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Kusama is not the only user of these converters, parachains will also import these into their xcm configurations from xcm-builder. I believe this also responds the previous comment.

Copy link
Member

@ggwpez ggwpez Jun 28, 2023

Choose a reason for hiding this comment

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

About the previous comment: the TINKERNET_PARA_ID is only valid on Kusama, right? So could be named KUSAMA_ TINKERNET_PARA_ID.

@paritytech-ci paritytech-ci requested a review from a team June 28, 2023 08:40
@kianenigma
Copy link
Contributor

bot rebase

@paritytech-processbot
Copy link

Rebased

@ggwpez
Copy link
Member

ggwpez commented Jun 28, 2023

bot merge

@paritytech-processbot
Copy link

Waiting for commit status.

@paritytech-processbot paritytech-processbot bot merged commit 52595c8 into paritytech:master Jun 28, 2023
46 checks passed
@arrudagates
Copy link
Contributor Author

🎉

gavofyork added a commit that referenced this pull request Jun 30, 2023
@gavofyork
Copy link
Member

gavofyork commented Jun 30, 2023

Items specific to Kusama which go into common crates like xcm-builder should:

a) be placed in a Kusama-specific path or have "Kusama" clearly placed in its name; and
b) convert the MultiLocation value to be universal and then that used for matching.

This code makes little sense outside of the very specific context it is designed for. In particular, matching code like parents: _ is not tight enough, especially when combined with a non-consensus-specific Parachain junction matcher. First converting into a universal location and then matching on that would alleviate these issues.

Once the runtimes are moved out into the Fellowship repo, this code should be, too. For now it can reasonably live next to the runtime, but I think it's just too inherently opinionated to be placed in xcm-builder.

@ggwpez ggwpez added B0-silent Changes should not be mentioned in any release notes and removed B1-note_worthy Changes should be noted in the release notes labels Jun 30, 2023
paritytech-processbot bot pushed a commit that referenced this pull request Jun 30, 2023
@arrudagates
Copy link
Contributor Author

b) convert the MultiLocation value to be universal and then that used for matching.

@gavofyork From what I understand, to do this I need to send a UniversalOrigin instruction with a GlobalConsensus junction, which will then mutate the origin similarly to how DescendOrigin works. But then I hit an immediate blocker, as this instruction is currently unsupported in Kusama/Rococo (which was very hard to figure out, since the way it's made unsupported is by giving it max weight and no meaningful logs): https://github.com/paritytech/polkadot/blob/master/runtime/kusama/src/weights/xcm/mod.rs#L242

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. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants