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

feat: align LocationToAccountId derivation #67

Conversation

mustermeiszer
Copy link
Contributor

In order to allow other chains to have consistent "remote" accounts in other chains, it is needed that all chains in the ecosystem derive accounts in the XCM logic in the same way.

See https://forum.polkadot.network/t/multichain-friendly-account-abstraction/1298/23

This PR aligns all runtimes of system-parachains to have that derivation. It is debatable whether this derivation should also go into the relay-chain runtimes.

@mustermeiszer
Copy link
Contributor Author

mustermeiszer commented Oct 19, 2023

The formatting changes are due to running rustfmt on the changed files. Not sure, but it seems they are not formatted according to this repos fmt rule in the first place, or my formatter is doing something odly.

Nevermind, ran it with +nightly --all

@mustermeiszer
Copy link
Contributor Author

Ping @KiChjang

@KiChjang
Copy link
Contributor

KiChjang commented Nov 8, 2023

Why wouldn't you want to do this on relay chain runtimes?

@mustermeiszer
Copy link
Contributor Author

Why wouldn't you want to do this on relay chain runtimes?

I would like to have that on the relay too. Just thought, this would give more backslash.

@mustermeiszer
Copy link
Contributor Author

@KiChjang what is needed to get this closed and when could we expect this to be live in the networks?

@KiChjang
Copy link
Contributor

KiChjang commented Nov 8, 2023

I'm only a rank 2 member, so I can't actually approve and merge PRs that touch runtime files. According to the rules set here, you'll want to grab the attention of 4 members with a rank of 3 or higher to approve and merge this.

@mustermeiszer
Copy link
Contributor Author

cc @gavofyork

@mustermeiszer
Copy link
Contributor Author

cc @xlc @bkchr @rphmeier @shawntabrizi

@rphmeier
Copy link
Contributor

rphmeier commented Nov 10, 2023

@mustermeiszer thanks for the ping.

It looks like this is attempting to solve a similar problem to https://github.com/polkadot-fellows/RFCs/pull/34/files which has just been redrafted by @arrudagates . I'm not deep into the details of XCM but at an initial review they seem orthogonal. Let's discuss, but in case RFC 34 is conflicting and more suitable here, we should look to get that approved and then apply here rather than merging this change in the meantime and then changing it.

@mustermeiszer
Copy link
Contributor Author

mustermeiszer commented Nov 20, 2023

Thanks for the info and message @rphmeier and sorry that it took me 10 days to answer. Takes more focus and time than I always expect when trying to understand what actually is happening underneath in XCM.

I asked my questions in the RFC polkadot-fellows/RFCs#34 (comment). Nevertheless, as we have already deployed the previous supported approach, we kind of have already decided to not support that. I do not really see a way to support both approaches in the ecosystem and we are looked IIUC.

As you said, they seem orthogonal.

@mustermeiszer
Copy link
Contributor Author

@rphmeier rfc polkadot-fellows/RFCs#34 (comment) seems to be stalling a bit. What would be your proposed way forward here?

Kusama Asset-Hub already has the proposed derivation scheme in production (see), of course arguing for my own case here. ^^

In general, we need clarity about the derivation scheme in the ecosystem, this would help us a lot ensuring higher security for large token transfers as we can act all the time from our own parachain, customizing the behaviour we want there.

@mustermeiszer
Copy link
Contributor Author

Sorry for pinging again. I would really appreciate a convo on this. Our assumption at Centrifuge was that this PR made by @gavofyork would be the way forward to have a general purpose account derivation scheme in xcm, so that parachain users can have safe native-like accounts on all other chains in the ecosystem.

It would be really beneficial to have the same derivation on the system parachains so that we can start building logic for that. Given that Kusama Asset-Hub already supports the here proposed derivation I would argue having the same derivation in the other chains makes sense.

Copy link
Contributor

@acatangiu acatangiu left a comment

Choose a reason for hiding this comment

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

AFAICT this PR is a non-controversial subset of polkadot-fellows/RFCs#34. Changes themselves are correct and not doing anything controversial.

Review rules have recently changed.
@mustermeiszer you should rebase, get latest main and rules.
@KiChjang your review should also count afterwards, you might need to submit it again though.

@mustermeiszer mustermeiszer force-pushed the feat/align-xcm-account-derivation branch 3 times, most recently from 616cf03 to c182773 Compare December 6, 2023 14:28
@mustermeiszer
Copy link
Contributor Author

@acatangiu thanks for the feedback and comment! PR is rebased now.

@mustermeiszer
Copy link
Contributor Author

@rphmeier according to the rfc statement, there seems to be no conflict between the rfc and this pr.

Under these circumstances do you agree with the approvals here? I would like to merge this, so we can move forward and implement using this derivation.

@mustermeiszer mustermeiszer force-pushed the feat/align-xcm-account-derivation branch from c182773 to ffae502 Compare December 12, 2023 15:42
@mustermeiszer
Copy link
Contributor Author

Rebased and ready to go in: @sorpaas @KiChjang @xlc @acatangiu @shaunxw @rphmeier @gavofyork

@bkchr
Copy link
Contributor

bkchr commented Dec 12, 2023

/merge

@fellowship-merge-bot fellowship-merge-bot bot enabled auto-merge (squash) December 12, 2023 16:36
@fellowship-merge-bot
Copy link
Contributor

Enabled auto-merge in Pull Request

Available commands
  • /merge: Enables auto-merge for Pull Request
  • /merge cancel: Cancels auto-merge for Pull Request
  • /merge help: Shows this menu

For more information see the documentation

fellowship-merge-bot bot added a commit that referenced this pull request Feb 2, 2024
We imported the runtimes into this repo at tag
[`v1.1.0-rc2`](https://github.com/paritytech/polkadot-sdk/commits/v1.1.0-rc2/)
but still merged stuff until they got removed in
[#1731](paritytech/polkadot-sdk#1731).
This re-applies all changes in that commit range
[`v1.1.0-rc2..bf90cb0b`](paritytech/polkadot-sdk@v1.1.0-rc2...bf90cb0)
checked in the SDK with
`git log v1.1.0-rc2..bf90cb0b -- polkadot/runtime/`. Closes
#112

Most changes got applied as part of
#56, but two were
missing:
- paritytech/polkadot-sdk#1303
- <s>https://github.com/paritytech/polkadot-sdk/pull/1476</s> (reverted)

<s>One [open
Q](#67 (comment))
since there was a difference between
[#1291](paritytech/polkadot-sdk#1291) and
#67.</s>

---------

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: fellowship-merge-bot[bot] <151052383+fellowship-merge-bot[bot]@users.noreply.github.com>
Co-authored-by: Bastian Köcher <git@kchr.de>
Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants