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 Absolute Location Account Derivation #34

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

arrudagates
Copy link

@arrudagates arrudagates commented Oct 5, 2023

This RFC proposes the introduction of Tinkernet's multisig XCM configs to both Kusama and the Kusama Asset Hub. These configs are spcifically a MultiLocation to AccountId converter and a MultiLocation to Origin converter. This enables the multisigs managed by the Tinkernet Kusama parachain to operate under Kusama and Asset Hub using their proper accounts, which are derived locally using a static derivation function with the goal to preserve the same account address across the origin and any destination chains, hence the need for the converter configs to be implemented directly in the receiver chains.

The draft PR implementing this proposal can be found here: polkadot-fellows/runtimes#52

For more technical information about the Saturn protocol, please refer to this Polkadot Forum post: https://forum.polkadot.network/t/saturn-xcm-multisig-integration-on-kusama-a-technical-discussion/2694

Higher level information about the Saturn protocol can be found in this article: https://invarch.medium.com/saturn-the-future-of-multi-party-ownership-ac7190f86a7b

These resources and more information can also be found in the proposal.

After discussions this RFC has been rewritten to achieve the same goal but in a generic way that does not involve introducing any custom configs tailor-made for InvArch/Tinkernet and instead provides a solution usable by any ecosystem developer.

@gavofyork
Copy link
Contributor

This opens a wider question - to what extent should system chains allow modification and increased complexity (with accordingly higher maintenance costs and attack surface) in order to introduce additional compatibilities with private interests.

Thus far, my assumption has been that private interests should really be the ones to alter themselves in order to be compatible with system chains. Where no possibility of compatibility exists due to a lack of functionality on system chains, then it seems to me to be reasonable that functionality which is neutral and agnostic to third-party interests should be introduced, but not functionality specific to any one third-party.

This RFC would not appear to be in line with that.

@lnvArchitect
Copy link

@gavofyork, from a philosophical point of view, I understand where you are coming from; but I believe that your underlying concerns, specifically the problems they entail, are not sufficiently reflected by the product of this specific RFC.

To talk about the broader concept concerning changes to system changes, and make a single stance that prevents any possibilities is one thing. But rather than absolutely prevent any threats and successes that could come from such, I believe it's better, to approach these on a case-by-case basis. At the least, do so when the RFC is submitted by a Parachain representative, which I assume would limit the number of such RFCs, resulting in a (likely) even smaller amount getting accepted, and may or may not serve to benefit the broader ecosystem as a whole (just as a systems chain does).

In the case of this specific RFC, I would argue that it poses no threat to a system chain. I believe this modification is hardly an attack vector and, in theory, no different than any smart contract launched on a network. It adds functionality, but it doesn’t force its use, that’s a decision determined by free people. In this specific case, the RFC does not remove or alter any existing code, but adds our configs which consist of very few lines of code with a single clear end result.

At the very least, and considering my (hopefully understandable) previous words, we would like to differentiate between the Polkadot & Kusama ecosystems & ask that we be given the ability to showcase & hopefully validate the network effect that we believe this RFC will achieve.

A single account, whether for a multisig or a DAO, to exist as a first class citizen across any chain, that's a huge thing uniquely possible only using this tech stack. We understand an RFC concerning the Polkadot relay will be a different matter of its own, but assuming you otherwise believe the code itself looks good (Gabe made your suggested changes following your comment here), then we would appreciate the opportunity to show-off what we built on Kusama & funded by its treasury - really cool shit.

@lnvArchitect
Copy link

I also believe that, and agree that your view regarding system chain reliance should typically be considered as a "best practice," but this is a risk the Tinkernet Parachain is taking on; compared to a system chain being reliant on a private chain, which I assume would be more controversial. This is also a unique need due to our focus on account abstraction, as we imagine custom XCM configs not being necessary for most other areas of innovation.

This is necessary for us, and even in the event of a critical systems chain issue, this would impact UX no different than trying to send funds from an exchange to Solana (or vise-versa) when it's down; inconvenient, but funds are safe, as long as the relay is producing blocks, the overall service remains resilient.

Finally, I understand that Tinkernet is technically a private entity, but our entire focus has always been centered around the value of our network being dependent on the value creation it drives on other parachains. So yes, this benefits our interests, but those interests are benefiting the entire ecosystem. They're intentionally intertwined. We're even open to the idea of an eventual Polkadot - acquisition -> Parachain - convert -> System Chain; an idea I've pitched to Rob on more than one occasion this past year.

@rphmeier
Copy link
Contributor

Where no possibility of compatibility exists due to a lack of functionality on system chains, then it seems to me to be reasonable that functionality which is neutral and agnostic to third-party interests should be introduced, but not functionality specific to any one third-party.
This RFC would not appear to be in line with that.

This makes sense on the face of it, but is there a clear alternative or generalization which can be made in XCM to accommodate this use-case? I dig in a bit below, but would appreciate some context from XCM experts.

using a static derivation function with the goal to preserve the same account address across the origin and any destination chains, hence the need for the converter configs to be implemented directly in the receiver chains.

The primary goal here is the user experience of observing the same account ID derived on each chain. This seems quite useful for the legibility of multi-chain interactions, as it removes the need to keep an index and mapping (likely maintained by some third party) of all the account IDs on all the chains.

This RFC seems to be a workaround for the fact that there is no generally available way in XCM to derive the same account ID on all chains. I would like to see some generalization here, and the root of the problem seems to be the HashedDescriptionOf<DescribeFamily> used to derive remote account IDs from XCM origins giving different results based on whether the XCM originates from a child or sibling, regardless if the expressed interior location remains the same.

Unfortunately, any modification to DescribeFamily implementation would break existing backwards compatibility, which brings in a need for special-casing if consistency across all types of relationships is desired. I don't believe ergonomics factored in as a main consideration in the design of this interface.

@arrudagates - XCM isn't my focus area, but my assessment is that:

  1. The current XCM config can currently be used to yield the exact same AccountID32 on all parachains which are direct siblings - including all system chains, TinkerNet, and all market-allocated parachains.
  2. it does not allow for the same account ID to be derived on the relay chain, which is a parent, or on remote locations, such as over bridges.
  3. Changing this requires either special-casing or breaking backwards compatibility in account derivation on the relay networks.

I hope that the effect of (2) is diminished by #32 and also believe that (1) gets TinkerNet most of the way.

I also think it is unfortunate that this work has been subject to so much whiplash and we should strive for a less capricious contributor experience.

@tomaka
Copy link
Contributor

tomaka commented Oct 14, 2023

I know basically nothing about XCM, but if my understanding is correct I agree with the other comments

In the case of this specific RFC, I would argue that it poses no threat to a system chain. I believe this modification is hardly an attack vector and, in theory, no different than any smart contract launched on a network. It adds functionality, but it doesn’t force its use, that’s a decision determined by free people.

You haven't answered the most critical problem: the increased complexity.

The reason why we have a specification and RFCs is for Polkadot as a whole (its protocols, system chains, etc.) to be self-scoped and precisely defined.

If you add a concept named "TinkerNet multisig" to the specification (the specification that exists only in theory but still exists nonetheless), you would need to precisely define every relevant aspect of what a "TinkerNet multisig" is, and you would need to submit RFCs if you ever want to modify one of these aspects.
Otherwise if you add a concept to Polkadot whose details are controlled by a private third party, you effectively introduce a backdoor into Polkadot.

@sourabhniyogi
Copy link

sourabhniyogi commented Oct 14, 2023

3. Changing this requires either special-casing or breaking backwards compatibility in account derivation on the relay networks.

In trying out remote execution patterns, I found the 1.0 XCM Transact's dependence on derivative accounts to be ergonomically unusable by every day users and extremely challenging for developers to use across chains. By comparison, LayerZero is easy to understand for users and developers alike:

https://layerzero.gitbook.io/docs/evm-guides/master/how-to-send-a-message

(where messages contain signed transactions complete with nonces, timestamps, etc.). Rather than breaking backwards compatibility, is there a 2.0 era new instruction like XCM Transact2 (renamed suitably) design that can skip all the derivative account complexity using "system wide accords" and recognize that feature matching Layerzero's usability is an actual product-level goal worthy of achieving? I do not know how CoreJam and XCM will feel, but if it involves derivative accounts still we really need to start over still... If you are able to get the same derivative account on all other chains (sibling/parent/child), is there nothing we can do to remove it altogether?

If we need nonces to win usability, I want that. I don't think the "we prioritize security over usability" mantra should imply that you end up with an unusable XCM where LayerZero wins. There should be a happy medium where both security and usability concerns are addressed and 2.0 has a competitive product that smarter than the 1.0 design that came before it. I think it would be ok to be breaking if you win this.

@gavofyork
Copy link
Contributor

Now that we have the concept of a universal location, I see no great problem with introducing a new form of DescribeFamily which uses the universal (absolute) location rather than the usual (relative) location. If this becomes an agreed-on standard, then providing an XCM endpoint doesn't alter its universal location, then the address hash should be the same regardless of the chain on which it is evaluated.

@arrudagates
Copy link
Author

@rphmeier

This RFC seems to be a workaround for the fact that there is no generally available way in XCM to derive the same account ID on all chains. I would like to see some generalization here, and the root of the problem seems to be the HashedDescriptionOf<DescribeFamily> used to derive remote account IDs from XCM origins giving different results based on whether the XCM originates from a child or sibling, regardless if the expressed interior location remains the same.

While generalization for this use case would certainly be good, I don't think it should be a blocker for this RFC, specially considering this doesn't introduce or impact any existing functionality for relay/asset hub users, instead what it does is open the door for Tinkernet multisig users to use the relay and asset hub.

I also think it is unfortunate that this work has been subject to so much whiplash and we should strive for a less capricious contributor experience.

There will always be many different ways to achieve the same results, I do understand wanting to strive for a path that pleases everyone, but for something that has a minor impact on ergonomics yet still achieves the same outcome, this has turned into a very long process, specially considering it was already approved and merged at one point.

@lnvArchitect
Copy link

Replying to @tomaka:

The reason why we have a specification and RFCs is for Polkadot as a whole (its protocols, system chains, etc.) to be self-scoped and precisely defined.

Gabe originally submitted a PR seeking to achieve the goals explained in this RFC starting on May 2nd, over 5 months back, where the PR was merged after being reviewed & approved by individuals such as Rob Habermeier, Keith Yeung, & Kian Paimani, many of which are in the fellowship & are voting members. However, the PR merge was reverted by @gavofyork, where he left feedback & highlighted that this should be handled under the Fellowship repo in the future, which can be found here. After acting on the feedback given & waiting for the Fellowship repo to be available, Gabe submitted a new PR here, where he was advised by Gav that this should be submitted as an RFC.

If you add a concept named "TinkerNet multisig" to the specification (the specification that exists only in theory but still exists nonetheless), you would need to precisely define every relevant aspect of what a "TinkerNet multisig" is, and you would need to submit RFCs if you ever want to modify one of these aspects.

It's important to understand that this doesn't change the relay architecture at all. This RFC does not add any externally controlled functionality to the relay.

Otherwise if you add a concept to Polkadot whose details are controlled by a private third party, you effectively introduce a backdoor into Polkadot.

Users would not go to the Kusama relay chain or Asset Hub to generate a Saturn Multisig account, powered by Tinkernet. This RFC simply allows Tinkernet multisig accounts to transact across Kusama & Asset Hub as first-class citizens.

@sourabhniyogi
Copy link

sourabhniyogi commented Oct 16, 2023

@arrudagates @XCAstronaut I am so glad you submitted this RFC because it addresses a central point of contention in the ecosystem. Would you be so kind so as to transform the nature and scope of this RFC to not be about TinkerNet (or Kusama + AssetHub) alone narrowly but to lead the way on making an ecosystem wide solution? I would hope you could follow through on @gavofyork's suggestion on "an agreed-on standard" ala HashedDescriptionOf<DescribeFamily>, where "the address hash should be the same regardless of the chain on which it is evaluated" as the target to meet. This is the way, because it would be ecosystem-wide, and very important because it addresses XCM ergonomics/usability head on.

I would hope to see it cover Moonbeam + Astar such that XCM Transact is as developer friendly and user friendly as we can possibly make it. Getting the input from those 2 teams would support the EVM+Substrate remote execution case and I'm sure AssetHub's central use case would start humming within Kusama + Polkadot. The end result, as I understand it, would be that every Substrate+EVM address would have at most 1 (ok at most 2) derived accounts and the reasoning about BridgeHub <> Ethereum would be made explicit. If you can't take the lead singlehandedly, Albertov19+Gorka of Moon{...}/Tanssi, Dino Pacandi of Astar (+ Vincent Geddes of Snowfork?) would be excellent collaborators and are keenly aware of everything going on on derived accountIDs.

Thank you so much for leading the way on this!

@arrudagates
Copy link
Author

@sourabhniyogi From your previous messages I believe you misunderstand what this RFC is about, in your first comment you suggest sending signed data over XCM, which is impossible here as these accounts don't have private keys. I suggest you take a look at the two links provided in the PR description.

@sourabhniyogi
Copy link

sourabhniyogi commented Oct 16, 2023

@sourabhniyogi From your previous messages I believe you misunderstand what this RFC is about, in your first comment you suggest sending signed data over XCM, which is impossible here as these accounts don't have private keys. I suggest you take a look at the two links provided in the PR description.

I do wish for signed data, but the next best thing is at most 1 (or 2, covering Substrate+EVM) derived "keyless" accounts which is a massive improvement over not having a standard at all. While I wish there would be no "keyless" derived accountIDs at all, this next best thing is OK and a considerable improvement in usability, which I think we will have to settle for.

@rphmeier
Copy link
Contributor

Now that we have the concept of a universal location, I see no great problem with introducing a new form of DescribeFamily which uses the universal (absolute) location rather than the usual (relative) location. If this becomes an agreed-on standard, then providing an XCM endpoint doesn't alter its universal location, then the address hash should be the same regardless of the chain on which it is evaluated.

@gavofyork Thanks - this seems to provide a general solution

@arrudagates
Copy link
Author

arrudagates commented Oct 18, 2023

@gavofyork

Now that we have the concept of a universal location, I see no great problem with introducing a new form of DescribeFamily which uses the universal (absolute) location rather than the usual (relative) location. If this becomes an agreed-on standard, then providing an XCM endpoint doesn't alter its universal location, then the address hash should be the same regardless of the chain on which it is evaluated.

The current implementation of the WithComputedOrigin barrier returns relative locations for origins containing the GlobalConsensus junction, so if I understand correctly we would need to change the barrier like this (rough example):

UniversalOrigin(new_global) => {
-    // Note the origin is *relative to local consensus*! So we need to escape
-    // local consensus with the `parents` before diving in into the
-    // `universal_location`.
-    actual_origin = X1(*new_global).relative_to(&LocalUniversal::get());

+    // Alternatively we could pass the relay universal to the barrier instead of the local universal.
+    // Then we wouldn't need this if let statement.
+    if let Ok(this_global) = LocalUniversal::get().global_consensus() {
+        if *new_global != this_global.into() {
+            return Err(ProcessMessageError::Unsupported);
+        }
+
+        actual_origin = MultiLocation::grandparent()
+            .pushed_front_with_interior(this_global)
+            .map_err(|_| ProcessMessageError::Unsupported)?
+            .appended_with(actual_origin)
+            .map_err(|_| ProcessMessageError::Unsupported)?;
+    }
}

https://github.com/paritytech/polkadot-sdk/blob/master/polkadot/xcm/xcm-builder/src/barriers.rs#L184-L189

From what I understand of absolute/universal locations, this is what the barrier would have to look like to actually return the proper absolute location in both the relay and parachains for messages prefixed with the UniversalOrigin instruction. Then with that out of the way we can write DescribeFamily location converters for absolute locations.

Is this correct or am I missing something?

@gavofyork
Copy link
Contributor

Yes, the converter impl must have a type param to inform it of the system's universal location (which I presume here is LocalUniversal).

@arrudagates
Copy link
Author

I rewrote this RFC as a generic solution using absolute locations through UniversalOrigin. This new RFC proposes the changes necessary to the WithComputedOrigin barrier and DescribeFamily MultiLocation descriptor to achieve the end goal of deriving the same account across all chains.

@arrudagates arrudagates changed the title Introduce Tinkernet XCM configs to Kusama and Asset Hub XCM Absolute Location Account Derivation Oct 28, 2023
@mustermeiszer
Copy link

Sorry, for asking even more questions, but having an aligned account derivation accross the ecosystem is extremly important for us at Centrifuge.

Summarising, the goal of this RFC is to allow for a unique but uniform account derivation for users indepent from which chain a user executes their XCM, right? Just for me to not already start with a wrong assumption.


Under this assumption, I have a few questions. As I am in no way an expert on XCM please excuse if any of them are wrong or easily answerable:

@mustermeiszer
Copy link

One more thing I need some help understanding.

  • To me it seems like this means that chains need to trust all other chains to eleviate the origin to a UniversalOrigin

is this correct, as otherwise the executor will fail on this line if I am not mistaken.

@vgeddes
Copy link

vgeddes commented Nov 21, 2023

  • Derivation would not work for XCM comming over a bridge, right? E.g. BridgeHub elevating the origin as coming from Ethereum as Snowbridge is planning to do.

Indeed, yes, when contracts (and EOAs) on Ethereum want to transact with parachains, BridgeHub will end up forwarding the following message to the destination parachain:

UniversalOrigin(GlobalConsensus(Ethereum))
DescendOrigin(AccountKey20(...))
Transact(...)

So any location converter wishing to support this would need to handle origin locations of the form /GlobalConsensus(Ethereum)/AccountKey20.

@mustermeiszer
Copy link

@arrudagates would appreciate if you find the time to answer my questions above?

I think although having worse UX, the approach that @gavofyork proposed in this PR should be favored. The problem is that a user will not have a the same account on all other chains, but at least we have a common derivation scheme.

For UX chains should IMO provide a runtime-api that takes a Multilocation and returns the derived address if possible (Example see here).

I am not sure, how the fellowship wants to move forward here, but this RFC seems to be blokcing this PR which is just aligning the account derivation of the runtimes to the one already deployed to Kusama Asset-Hub

@arrudagates
Copy link
Author

arrudagates commented Dec 6, 2023

@arrudagates would appreciate if you find the time to answer my questions above?

I think although having worse UX, the approach that @gavofyork proposed in this PR should be favored. The problem is that a user will not have a the same account on all other chains, but at least we have a common derivation scheme.

For UX chains should IMO provide a runtime-api that takes a Multilocation and returns the derived address if possible (Example see here).

I am not sure, how the fellowship wants to move forward here, but this RFC seems to be blokcing this PR which is just aligning the account derivation of the runtimes to the one already deployed to Kusama Asset-Hub

There seems to be some misunderstanding about the proposed changes in this RFC, this does use the APIs introduced in PR 7329 and it does not block PR 67 as the changes proposed are to the types mentioned in the PR (HashedDescription/DescribeFamily), however the changes should be backwards compatible and only expand on what DescribeFamily can describe, rather than changing any of the existing match arm cases.

What I mean by this is that this RFC is not only compatible with your PR 67, but your proposed change is actually one of the required steps to implement this RFC.

@arrudagates
Copy link
Author

I made changes to the RFC text again, this time hopefully addressing all concerns regarding implementation details and out of scope details. Please take a read and feel free to provide feedback or submit a review!

@gavofyork
Copy link
Contributor

gavofyork commented Dec 15, 2023

This mentions that two trait impls found in XCM builder must be changed but it does not state specifically how they should be changed, why these two are special, why no other extensions built on XCM-executor (inside and outside of Polkadot-SDK) would need altering, why compatibility will be preserved and all existing systems will not break, nor, most importantly, what foundational API assumptions change.

Copy link
Contributor

@gavofyork gavofyork left a comment

Choose a reason for hiding this comment

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

...


This proposal aims to make it possible to derive accounts for absolute locations, enabling protocols that require the ability to maintain the same derived account in any runtime. This is done by deriving accounts from the hash of described absolute locations, which are static across different destinations.

The same location can be represented in relative form and absolute form like so:
Copy link
Contributor

Choose a reason for hiding this comment

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

This only makes sense to say when the local location (in terms of the universal origin) is defined.

Edit: I see now it is defined below. I'd move it up here instead.

Using `DescribeFamily`, the above relative locations would be described like so:
```rust
// Relative location (from own perspective)
// Not possible.
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be manageable with DescribeTerminus

@gavofyork
Copy link
Contributor

This is basically the code I would add to builder.

pub struct AsUniversal<Inner, Context>(PhantomData<(Inner, Context)>);
impl<
	Inner: DescribeLocation,
	Context: Get<InteriorMultiLocation>,
> DescribeLocation for AsUniversal<Context> {
	fn describe_location(location: &MultiLocation) -> Option<Vec<u8>> {
		let universal = location.clone().reanchored(&MultiLocation::default(), Context::get());
		Some((b"universal:", Inner::describe_location(universal)?).encode())
	}
}

Now, any chain which places this around their describe-location code will get the same account ID regardless of where they are in the ecosystem. It will work for subsets of locations too, so you can have a tuple like type DescribeLocation = (LegacyDescribeLocation, AsUniversal<CommonLocations, MyLocation>);. This is important, as any location already in use in a chain would end up with a different ID if it were placed in this wrapper.

Chains need to be careful when altering their DescribeLocation definition. Any locations already holding local assets would lose access to their assets if DescribeLocation definition changed so their account ID for their location become different.

Since DescribeLocation is already in use on both the Relay-chain and parachains and these are at different levels of the ecosystem location graph, there is fundamentally no way to make the "common-account-ID" work ecosystem-wide without causing intractible breakage.

It would therefore need to be an opt-in thing for chains, or--possibly--the XCM language itself could be altered to support alternative LocationConversions, but this would be a pretty major undertaking.

@arrudagates
Copy link
Author

@gavofyork I replied on Matrix but will copy the messages here too:

The RFC proposes changing specifically the WithComputedOrigin barrier and the DescribeFamily struct, changing these would not mess with already in-use location derivations as we would simply add another arm branch to DescribeFamily for [GlobalConsensus(...), ...]

This is not meant to be complicated or foundational at all, everything is already in place for this, it just needs a couple missing pieces.

The intended flow is for an xcm containing a UniversalOrigin instruction to arrive, pass through WithComputedOrigin with the modified origin being [GlobalConsensus(...), ...], then this origin goes to DescribeFamily and matches a new arm that describes it as an absolute location. There is nothing breaking in this and it shouldn't need a whole new struct for runtimes to opt-in.

@arrudagates
Copy link
Author

@gavofyork I replied on Matrix but will copy the messages here too:

The RFC proposes changing specifically the WithComputedOrigin barrier and the DescribeFamily struct, changing these would not mess with already in-use location derivations as we would simply add another arm branch to DescribeFamily for [GlobalConsensus(...), ...]

This is not meant to be complicated or foundational at all, everything is already in place for this, it just needs a couple missing pieces.

The intended flow is for an xcm containing a UniversalOrigin instruction to arrive, pass through WithComputedOrigin with the modified origin being [GlobalConsensus(...), ...], then this origin goes to DescribeFamily and matches a new arm that describes it as an absolute location. There is nothing breaking in this and it shouldn't need a whole new struct for runtimes to opt-in.

@gavofyork I wrote a proof of concept of these changes, including a demo of the resulting account derivation and proof that it won't affect compatibility with code that's already in use.
https://github.com/arrudagates/RFC-34-test

➜  rfc-34-test git:(master) cargo run
   Compiling rfc-34-test v0.1.0 (/Users/gabe/Documents/tmp/rfc-34-test)
    Finished dev [unoptimized + debuginfo] target(s) in 0.63s
     Running `target/debug/rfc-34-test`
Absolute account from perspective of the relay:
origin: MultiLocation { parents: 0, interior: X3(GlobalConsensus(Kusama), Parachain(2125), Plurality { id: Index(0), part: Voice }) }
account: 47abfeb0233b987f9b9804ee365965b32dfae135aee5ca5f16c0df299e7867f2 (5DggMG5A...)

Absolute account from perspective of a para:
origin: MultiLocation { parents: 0, interior: X3(GlobalConsensus(Kusama), Parachain(2125), Plurality { id: Index(0), part: Voice }) }
account: 47abfeb0233b987f9b9804ee365965b32dfae135aee5ca5f16c0df299e7867f2 (5DggMG5A...)

Proof the changes won't break "legacy":

"Legacy" AccountId32 origin:
origin: MultiLocation { parents: 0, interior: X2(Parachain(2125), AccountId32 { network: None, id: [2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2] }) }
account: e98cc99b08e4c6bd7b4a3398f095f077f1f2e77bb1a7c755c9eeb5ab637e8959 (5HLvqVg2...)

Post-RFC34 AccountId32 origin:
origin: MultiLocation { parents: 0, interior: X2(Parachain(2125), AccountId32 { network: None, id: [2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2] }) }
account: e98cc99b08e4c6bd7b4a3398f095f077f1f2e77bb1a7c755c9eeb5ab637e8959 (5HLvqVg2...)

"Legacy" Pallet origin:
origin: MultiLocation { parents: 0, interior: X2(Parachain(2125), PalletInstance(42)) }
account: 1b0e0567dccd3b2a29d3e52b33fc717c40559d7ee35b73bbc050f98039ea3c9f (5CgBKEYn...)

Post-RFC34 Pallet origin:
origin: MultiLocation { parents: 0, interior: X2(Parachain(2125), PalletInstance(42)) }
account: 1b0e0567dccd3b2a29d3e52b33fc717c40559d7ee35b73bbc050f98039ea3c9f (5CgBKEYn...)

"Legacy" terminal origin:
origin: MultiLocation { parents: 0, interior: X1(Parachain(2125)) }
account: c7e2b9edc94e50b0e65ebfc99bf8c7a254e2d25ac531156df1b7646ca03c173f (5Ganjkrd...)

Post-RFC34 terminal origin:
origin: MultiLocation { parents: 0, interior: X1(Parachain(2125)) }
account: c7e2b9edc94e50b0e65ebfc99bf8c7a254e2d25ac531156df1b7646ca03c173f (5Ganjkrd...)

@lnvArchitect
Copy link

lnvArchitect commented Dec 23, 2023

@gavofyork, I would greatly appreciate it if we could finally achieve some progress here. Gabe has been at this process for nearly 7 months now.

  1. He submitted a PR, which was reviewed & accepted by 5 people. At least 4/5 of those people are now fellowship members, at least 2/5 of them are now voting members, and one of them was even Rob, now a rank 6 member, like you. After 2 months (and a trip to Copenhagen) it was finally merged; however, less than 2 days later you reverted the merge.

  2. He submitted a second PR after making some adjustments, but also underscored that the suggestion you left on the previous PR did not appear feasible. Keith then closed the PR underscoring your point that the code in the PR being introduced to the xcm-builder directory was too (still) too opinionated.

  3. He submitted a third PR that defined the configs directly in two runtimes to solve the xcm-builder issue, where you then told him that it needed to be discussed as a Fellowship RFC first.

  4. He then submitted this RFC, where you then raised a point that anything that is (at least initially) in the interest of a specific parachain shouldn't be accepted, which was fair. So, Gabe made changes to achieve a general solution that technically allows any parachain to leverage the ability to power multichain accounts across ecosystem (whereas our interest is in solving account fragmentation for multisig & DAO accounts) in order to overcome your concern.

  5. After some weeks of nudging for attention on the RFC, Keith informed Gabe that the RFC needed to remove the pseudo code that elaborated on the RFC's intent & scope. After about another weeks or so of Gabe nudging about the RFC in the fellowship track, you told Gabe that he needed to address Keith's concerns asking him to cut back on his elaboration of the RFC's intent & scope, so he did exactly that.

  6. Gabe made more changes to the RFC, which he showed to Keith & made sure to first get his approval on. After a week, you then criticized the RFC for not elaborating enough, essentially contradicting the previous direction that you had requested from Gabe. You also go on express a misunderstanding of the RFC.

  7. You also express a misunderstanding of the RFC, and claim that (through the lens of your approach) the RFC would cause breaking changes; however, this is not correct. You expressed this incorrect assumption again on the recent Fellowship Call, claiming that what the RFC seeks to achieve is not possible, at least not without a much larger endeavor. Again, this is not true.

  8. Finally, Gabe has reverted back to elaborating on the RFC further to include proof of concept of the changes, including a demo of tests that prove what he has been saying - that this RFC & achieving a single account solution will not affect compatibility or cause any breaking changes within code that's already in use.

This enables a simple yet powerful feature using XCM for the ecosystem. It drastically reduces friction between chains for multisigs & DAOs. It can help realize a convenient solution for institutions to manage their assets across dozens (and one day thousands) of different parachains. It allows DAOs to transact as first class citizens across any parachain. It achieves a bridgeless asset management feature so that funds never need to leave any given chain. It's awesome.

Also, while yes, InvArch is already designed to leverage this feature to its fullest, it's not exclusive to InvArch. For example, rather than realizing cross chain practices over time in bits, some of which requiring their own unique amount of development effort I imagine, the Fellowship can leverage the functionality this RFC realizes to easily transact & execute call across any parachain that the Collectives parachain has a channel open with, which brings me to another point.

While the RFC seeks to enable a simple feature of XCM, granted a potentially powerful one, it is opt-in to any degree that I imagine could matter. Will InvArch be poised to leverage this feature to support multichain multisigs & DAOs? Yes. Will these accounts on InvArch suddenly have access to transact or manage assets on any parachain without their approval? No. The second half to realizing this system is to have open channels between parachains. If for whatever reason a parachain doesn't want to allow multichain DAOs or Multisigs powered by the InvArch network to transact on their network, then just don't open channels. However, whether it is for the convenience of InvArch or any other builder in the ecosystem, this RFC being accepted would make it so opening channels between chains is all that's necessary, versus opening channels & then adding custom XCM configs to a parachain. Still, if it's not desired, then it isn't forced. It's inherently opt-in.

Respectfully, you have been the only centralized source of friction here. While there being little to no documentation, source of direction, or precedent to reference, Gabe has consistently done what you have asked, addressed your concerns, and even gone out to show that this will not cause any breaking changes or harm to the relay & its parachains. If there are any explicit concerns over the RFC, then we are more than happy to solve them; however, no legitimate concern has been raised in nearly 2 months. We began the process of this RFC nearly 3 months ago, and this is starting to severely impact our roadmap. This functionality & feature for the ecosystem has been supported & funded by both the Kusama & Polkadot Treasuries via OpenGov, and with a supermajority of approval. This RFC is desired by not just Gabe or myself, and it is very stressful for us to have this be the only & final blocker when it shouldn't be one.

Again, there are no breaking changes & this not a foundational change, it's enabling a simple yet powerful feature, it does not exclusively benefit or is in the interest of any one parachain, and it is inherently opt-in regardless. If there is an explicit concern over the RFC, then we are more than happy to solve it. If there is specific information that you would like us to expand on, then we are happy to elaborate. Otherwise, we would very much appreciate if we can finally move forward on this, please.

@lnvArchitect
Copy link

@gavofyork, It has been 3 weeks since my last comment, and more than a week since the holiday period has ended. If you do not have any more feedback or issues with the RFC, then I would like to move forward on this. You are the sole bottleneck here, so it appears that nothing can happen without your approval. It's also unclear how to move forward without being dependent on you. I've reached out to 3 other voting members of the fellowship, and they, too, do not see any issues & have expressed their apologies to us for this experience. If there is something we are missing, can you please better explain?

Again, there are no breaking changes & this is not a foundational change; it's enabling a simple yet powerful feature; it does not exclusively benefit or is in the interest of any single parachain, and it is inherently opt-in regardless. If there is an explicit concern over the RFC, then we are more than happy to solve it. If there is specific information that you would like us to expand on, then we are happy to elaborate. Otherwise, we would very much appreciate it if we could finally move forward on this, please.

@gavofyork
Copy link
Contributor

gavofyork commented Jan 16, 2024

This is not meant to be complicated or foundational at all, everything is already in place for this, it just needs a couple missing pieces.

As soon as something is altered as low-level and broadly used as DescribeFamily and WithComputedOrigin, then the changes are necessarily foundational and must go through an exhaustive theoretical analysis. Writing some tests is a fine addition, but cannot be considered a comprehensive analysis.

Reinterpreting a relative location which begins with a GlobalConsensus node, regardless of its real interpretation (which would be informed by the number of parents) is a hack. It introduces the possibility of loss of funds or access to an account. The principle of least surprise implies it is probably not reasonable to introduce any unknown breakage on system chains or the Relay-chain. However it might reasonably be used on other parachains if their maintainers believe the benefits are worth the risk of breakage.

I believe the sensible way forward here would be to consider ways to introduce alternative location conversion mechanisms into XCM so that location conversion may be altered programatically and thus made to use absolute locations if desired.

@anaelleltd anaelleltd added the Stale Is no longer pursued. label Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Stale Is no longer pursued.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants