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

Use pallet instance in inbound queue origin #1033

Merged
merged 10 commits into from
Dec 2, 2023
Merged

Conversation

claravanstaden
Copy link
Contributor

Change to accomodate this requested change: paritytech/polkadot-sdk#2522 (comment) The EthereumInboundQueue pallet ID should form part of the origin to AssetHub.

Companion PR: Snowfork/polkadot-sdk#51

@@ -8,6 +8,8 @@
use frame_support::parameter_types;
use xcm::opaque::lts::NetworkId;

pub const INBOUND_QUEUE_MESSAGES_PALLET_INDEX: u8 = 80;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like there is an API for retrieving a pallet's index: https://substrate.stackexchange.com/a/794

Copy link
Contributor Author

@claravanstaden claravanstaden Dec 1, 2023

Choose a reason for hiding this comment

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

This works nicely in BridgeHub, but I need the EthereumInboundQueue pallet index on BridgeHub in the AssetHub runtime too: https://github.com/Snowfork/polkadot-sdk/pull/51/files#diff-a86375df98e04ca3cce1ea35c40257a222e2d5087f5f528ff33307678b78dc2dR863

I don't think it would be correct to import bridge hub as a dependency in the AssetHub crate, and I don't know how else to share the pallet index.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah ok makes sense 👌

Copy link

codecov bot commented Dec 1, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (312b05e) 81.50% compared to head (5ea29ab) 81.53%.
Report is 1 commits behind head on main.

❗ Current head 5ea29ab differs from pull request most recent head dc15e05. Consider uploading reports for the commit dc15e05 to get more accurate results

Files Patch % Lines
parachain/primitives/router/src/inbound/mod.rs 50.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1033      +/-   ##
==========================================
+ Coverage   81.50%   81.53%   +0.03%     
==========================================
  Files          54       54              
  Lines        2233     2237       +4     
  Branches       70       70              
==========================================
+ Hits         1820     1824       +4     
  Misses        398      398              
  Partials       15       15              
Flag Coverage Δ
rust 81.49% <50.00%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -165,6 +185,7 @@ where
BuyExecution { fees: xcm_fee, weight_limit: Unlimited },
// Fund the snowbridge sovereign with the required deposit for creation.
DepositAsset { assets: Definite(deposit.into()), beneficiary: bridge_location },
DescendOrigin(X1(PalletInstance(inbound_queue_pallet_index))),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this DescendOrigin be placed at the start of message, before BuyExecution?

As otherwise, we have three origins in this message, which makes it more complicated.

cc @alistair-singh

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This doesn't work, putting the DescendOrigin before BuyExecution then results in a barrier error. The way I understand it, the DescendOrigin shouldn't be before BuyExecution because it's not really related to the BuyExecution. BuyExecution is to pay for fees in DOT, and then the DescendOrigin is to further specialize the origin to be BridgeHub/Pallet(EthereumInboundQueue). @alistair-singh correct me if I am wrong.

Copy link
Contributor

@yrong yrong Dec 2, 2023

Choose a reason for hiding this comment

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

@claravanstaden I agree with @vgeddes here, IMO it's better to move DescendOrigin to first, add another PR with some revamp accordingly in
https://github.com/Snowfork/snowbridge/pull/1034/files#diff-9218e396410f01117c1c305b9cf006f6498bb0b3c0aedb8952a8450b4b1c5715 for us to review.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Awesome! Thanks Ron, taking a look...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @yrong, I merged it.

Copy link
Collaborator

@vgeddes vgeddes left a comment

Choose a reason for hiding this comment

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

+1

@claravanstaden claravanstaden marked this pull request as ready for review December 2, 2023 20:00
@claravanstaden claravanstaden merged commit c2142e4 into main Dec 2, 2023
7 checks passed
@claravanstaden claravanstaden deleted the use-inbound-pallet-id branch December 2, 2023 20:21
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.

3 participants