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.

Sign up for GitHub

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

Jump to bottom

Refactoring create asset and add CI tests to cover #1112

Closed
wants to merge 6 commits into from
Closed

Refactoring create asset and add CI tests to cover #1112

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1be0f8e
Runtime test for create asset in AH
yrong Jan 12, 2024
0aa33d0
Merge branch 'main' into ron/asset-hub-runtime-tests
yrong Jan 17, 2024
d085897
Move call index back to runtime
yrong Jan 17, 2024
37aa97e
Refactoring with CallInfo data structure
yrong Jan 17, 2024
25933c9
Merge branch 'main' into ron/asset-hub-runtime-tests
yrong Jan 22, 2024
474abe0
Merge branch 'main' into ron/asset-hub-runtime-tests
yrong Jan 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion .github/workflows/parachain.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,13 +233,20 @@ jobs:
repo-token: ${{ secrets.GITHUB_TOKEN }}
- name: setup rust toolchain
run: rustup show
- name: snowbridge runtime tests
- name: snowbridge runtime tests in bridge-hub
working-directory: polkadot-sdk
run: >
RUST_LOG=xcm=trace cargo test
--package bridge-hub-rococo-runtime
--test snowbridge
-- --nocapture
- name: snowbridge runtime tests in asset-hub
working-directory: polkadot-sdk
run: >
RUST_LOG=xcm=trace cargo test
--package asset-hub-rococo-runtime
--test snowbridge
-- --nocapture

integration-tests:
needs: check
Expand Down
10 changes: 2 additions & 8 deletions parachain/pallets/inbound-queue/src/mock.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,6 @@ const GATEWAY_ADDRESS: [u8; 20] = hex!["eda338e4dc46038493b885327842fd3e301cab39
parameter_types! {
pub const EthereumNetwork: xcm::v3::NetworkId = xcm::v3::NetworkId::Ethereum { chain_id: 11155111 };
pub const GatewayAddress: H160 = H160(GATEWAY_ADDRESS);
pub const CreateAssetCall: [u8;2] = [53, 0];
pub const CreateAssetExecutionFee: u128 = 2_000_000_000;
pub const CreateAssetDeposit: u128 = 100_000_000_000;
pub const SendTokenExecutionFee: u128 = 1_000_000_000;
Expand Down Expand Up @@ -252,13 +251,8 @@ impl inbound_queue::Config for Test {
type XcmSender = MockXcmSender;
type WeightInfo = ();
type GatewayAddress = GatewayAddress;
type MessageConverter = MessageToXcm<
CreateAssetCall,
CreateAssetDeposit,
InboundQueuePalletInstance,
AccountId,
Balance,
>;
type MessageConverter =
MessageToXcm<CreateAssetDeposit, InboundQueuePalletInstance, AccountId, Balance>;
type PricingParameters = Parameters;
type ChannelLookup = MockChannelLookup;
#[cfg(feature = "runtime-benchmarks")]
Expand Down
75 changes: 34 additions & 41 deletions parachain/primitives/router/src/inbound/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@ use sp_std::prelude::*;
use xcm::prelude::{Junction::AccountKey20, *};
use xcm_executor::traits::ConvertLocation;

const MINIMUM_DEPOSIT: u128 = 1;
pub const MINIMUM_DEPOSIT: u128 = 1;
pub const FOREIGN_CREATE_ASSET_WEIGHT_AT_MOST: Weight = Weight::from_parts(400_000_000, 8_000);

/// Messages from Ethereum are versioned. This is because in future,
/// we may want to evolve the protocol so that the ethereum side sends XCM messages directly.
Expand Down Expand Up @@ -83,24 +84,12 @@ pub enum Destination {
},
}

pub struct MessageToXcm<
CreateAssetCall,
CreateAssetDeposit,
InboundQueuePalletInstance,
AccountId,
Balance,
> where
CreateAssetCall: Get<CallIndex>,
pub struct MessageToXcm<CreateAssetDeposit, InboundQueuePalletInstance, AccountId, Balance>
where
CreateAssetDeposit: Get<u128>,
Balance: BalanceT,
{
_phantom: PhantomData<(
CreateAssetCall,
CreateAssetDeposit,
InboundQueuePalletInstance,
AccountId,
Balance,
)>,
_phantom: PhantomData<(CreateAssetDeposit, InboundQueuePalletInstance, AccountId, Balance)>,
}

/// Reason why a message conversion failed.
Expand All @@ -118,18 +107,9 @@ pub trait ConvertMessage {
fn convert(message: VersionedMessage) -> Result<(Xcm<()>, Self::Balance), ConvertMessageError>;
}

pub type CallIndex = [u8; 2];

impl<CreateAssetCall, CreateAssetDeposit, InboundQueuePalletInstance, AccountId, Balance>
ConvertMessage
for MessageToXcm<
CreateAssetCall,
CreateAssetDeposit,
InboundQueuePalletInstance,
AccountId,
Balance,
> where
CreateAssetCall: Get<CallIndex>,
impl<CreateAssetDeposit, InboundQueuePalletInstance, AccountId, Balance> ConvertMessage
for MessageToXcm<CreateAssetDeposit, InboundQueuePalletInstance, AccountId, Balance>
where
CreateAssetDeposit: Get<u128>,
InboundQueuePalletInstance: Get<u8>,
Balance: BalanceT + From<u128>,
Expand All @@ -150,10 +130,9 @@ impl<CreateAssetCall, CreateAssetDeposit, InboundQueuePalletInstance, AccountId,
}
}

impl<CreateAssetCall, CreateAssetDeposit, InboundQueuePalletInstance, AccountId, Balance>
MessageToXcm<CreateAssetCall, CreateAssetDeposit, InboundQueuePalletInstance, AccountId, Balance>
impl<CreateAssetDeposit, InboundQueuePalletInstance, AccountId, Balance>
MessageToXcm<CreateAssetDeposit, InboundQueuePalletInstance, AccountId, Balance>
where
CreateAssetCall: Get<CallIndex>,
CreateAssetDeposit: Get<u128>,
InboundQueuePalletInstance: Get<u8>,
Balance: BalanceT + From<u128>,
Expand All @@ -171,7 +150,6 @@ where

let owner = GlobalConsensusEthereumConvertsFor::<[u8; 32]>::from_chain_id(&chain_id);
let asset_id = Self::convert_token_address(network, token);
let create_call_index: [u8; 2] = CreateAssetCall::get();
let inbound_queue_pallet_index = InboundQueuePalletInstance::get();

let xcm: Xcm<()> = vec![
Expand All @@ -188,15 +166,14 @@ where
// Call create_asset on foreign assets pallet.
Transact {
origin_kind: OriginKind::Xcm,
require_weight_at_most: Weight::from_parts(400_000_000, 8_000),
call: (
create_call_index,
asset_id,
MultiAddress::<[u8; 32], ()>::Id(owner),
MINIMUM_DEPOSIT,
)
.encode()
.into(),
require_weight_at_most: FOREIGN_CREATE_ASSET_WEIGHT_AT_MOST,
call: Call::ForeignAssets(ForeignAssetsCall::create {
id: asset_id,
admin: MultiAddress::<[u8; 32], ()>::Id(owner),
min_balance: MINIMUM_DEPOSIT,
})
.encode()
.into(),
},
RefundSurplus,
// Clear the origin so that remaining assets in holding
Expand Down Expand Up @@ -318,3 +295,19 @@ impl<AccountId> GlobalConsensusEthereumConvertsFor<AccountId> {
(b"ethereum-chain", chain_id).using_encoded(blake2_256)
}
}

#[allow(clippy::large_enum_variant)]
#[derive(Encode, Decode, Debug, PartialEq, Eq, Clone, TypeInfo)]
pub enum Call {
/// Call create_asset on foreign assets pallet.
#[codec(index = 53)]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the pallet index should stay configured in the runtime config. What happens if the ForeignAssets pallet indexes are different per runtime? I think they are the same and that is the convention, but I still think this is runtime related and shouldn't live in the snowbridge crates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What happens if the ForeignAssets pallet indexes are different per runtime? I think they are the same and that is the convention.

Yeah, that's the precondition, checked that all the same in multiple runtimes(e.g. polkadot/kusama). I do not see a reason why they need to break this rule in the future so prefer to make things simple at this moment.

But I'm open if you all think that's a problem and will make some changes accordingly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can add a test to make sure they are in line?

Copy link
Contributor

@alistair-singh alistair-singh Jan 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ah I see a test is added. Ignore this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with Clara here, I am a bit uncomfortable with baking runtime configuration inside our pallets & primitives crate.

Secondly, I am sensitive about changing code unnecessarily at this point. Our next audit is scheduled for Feb 8, and this change will increase the scope of the audit.

I want to reserve any scope increases to accommodate for the second round of code review that the Parity bridges team is going to perform soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The initial intention of this PR is for the comment to add the call index to some primitives which can be referenced/checked in the AH runtime tests.

So @vgeddes do we still need this PR? I think we already have the emulated test register token to cover this use case so maybe the refactoring here and the unit test added in AH may not be necessary at all?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think Slava wanted us to move those constants here: polkadot-sdk/bridges/primitives/chain-asset-hub-rococo. I think it is fine to move the pallet index, but not to our crates. It should still be in the polkadot-sdk, and it should be configured per testnet. I think it can actually go here too: polkadot-sdk/cumulus/parachains/common/src/rococo.rs

Copy link
Contributor Author

@yrong yrong Jan 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks Clara and that makes sense! So do you think also better to move the hard-coded weight to some common runtime config?

Also noticed that Vincent mentioned to reserve any scope increases to accommodate for the second round of code review then not sure if it's the right time to do the refactoring.

So I'll convert the PR as draft and let the team decide whether to continue with the change or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@claravanstaden Some refactoring in 37aa97e as you suggested.

ForeignAssets(ForeignAssetsCall),
}

#[derive(Encode, Decode, Debug, PartialEq, Eq, Clone, TypeInfo)]
#[allow(non_camel_case_types)]
pub enum ForeignAssetsCall {
/// `pallet-assets::Call::create`
#[codec(index = 0)]
create { id: MultiLocation, admin: MultiAddress<[u8; 32], ()>, min_balance: u128 },
}
2 changes: 0 additions & 2 deletions parachain/primitives/router/src/inbound/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
use super::GlobalConsensusEthereumConvertsFor;
use crate::inbound::CallIndex;
use frame_support::parameter_types;
use hex_literal::hex;
use xcm::v3::prelude::*;
Expand All @@ -10,7 +9,6 @@ const NETWORK: NetworkId = Ethereum { chain_id: 11155111 };
parameter_types! {
pub EthereumNetwork: NetworkId = NETWORK;

pub const CreateAssetCall: CallIndex = [1, 1];
pub const CreateAssetExecutionFee: u128 = 123;
pub const CreateAssetDeposit: u128 = 891;
pub const SendTokenExecutionFee: u128 = 592;
Expand Down
2 changes: 1 addition & 1 deletion polkadot-sdk
Open in desktop
Submodule polkadot-sdk updated 2 files
+41 −0 cumulus/parachains/runtimes/assets/asset-hub-rococo/tests/snowbridge.rs
+2 −8 cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/lib.rs
Loading