Skip to content

Adapts XCM remote account derivation to match Polkadot suggestions#2344

Merged
crystalin merged 11 commits intomasterfrom
elois-xcm-derive-describe
Jun 14, 2023
Merged

Adapts XCM remote account derivation to match Polkadot suggestions#2344
crystalin merged 11 commits intomasterfrom
elois-xcm-derive-describe

Conversation

@librelois
Copy link
Copy Markdown
Collaborator

What does it do?

The intial goal of this PR was to cherry pick paritytech/polkadot#7329 on our polkadot fork and replace our custom non-standard hash derivation by the new type HashedDescription<AccountId, DescribeFamily<DescribeAllTerminal>>.

The problem is that it's not possible to cherry-pick paritytech/polkadot#7329 directly without performing a complete dependency upgrade. To get around this problem, I've manually rewritten just the part we're interested in: I've created a new type named HashedDescriptionDescribeFamilyAllTerminal that converts xcm multilocations into local accountId in the same way as HashedDescription<AccountId, DescribeFamily<DescribeAllTerminal>> does with the new traits.

To guarantee this equivalence, I wrote a rust test on our polkadot fork based on the master upstream branch: paritytech/polkadot@0679f2b.
Then I copied this test to our polkadot branch moonbeam-polkadot-v0.9.40: paritytech/polkadot@89fd916#diff-b210cb75f8786021ba64cb7a2377778668ce90a2c306f6a1a1b822a2bba61cfaR590

⚠️ Breaking Change ⚠️

The DescendOrigin instruction now derives accounts in a different way:

new behavior: blake2_256("SiblingChain" | paraId | "AccountKey20" | address)

old behavior: blake2_256("multiloc" | multilocation)

What important points reviewers should know?

Is there something left for follow-up PRs?

What alternative implementations were considered?

Are there relevant PRs or issues in other repositories (Substrate, Polkadot, Frontier, Cumulus)?

What value does it bring to the blockchain users?

@librelois librelois added B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. breaking Needs to be mentioned in breaking changes labels Jun 12, 2023
@librelois librelois changed the title Elois xcm derive describe XCM: Derive remote accounts in the same way as HashedDescription<AccountId, DescribeFamily<DescribeAllTerminal>> Jun 12, 2023
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Jun 12, 2023

Coverage generated "Tue Jun 13 19:35:08 UTC 2023":
https://s3.amazonaws.com/moonbeam-coverage/pulls/2344/html/index.html

Master coverage: 71.22%
Pull coverage: 71.22%

@librelois librelois marked this pull request as ready for review June 12, 2023 14:41
@crystalin crystalin mentioned this pull request Jun 8, 2023
24 tasks
Copy link
Copy Markdown
Contributor

@fgamundi fgamundi left a comment

Choose a reason for hiding this comment

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

LGTM

@crystalin crystalin merged commit d8bb06c into master Jun 14, 2023
@crystalin crystalin deleted the elois-xcm-derive-describe branch June 14, 2023 08:25
@crystalin crystalin changed the title XCM: Derive remote accounts in the same way as HashedDescription<AccountId, DescribeFamily<DescribeAllTerminal>> Adapt XCM remote account derivation to match Polkadot suggestions Jun 15, 2023
@crystalin crystalin changed the title Adapt XCM remote account derivation to match Polkadot suggestions Adapts XCM remote account derivation to match Polkadot suggestions Jun 15, 2023
@notlesh notlesh added D1-audited👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Aug 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes breaking Needs to be mentioned in breaking changes D1-audited👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants