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

Snowbridge v2 - Inbound Queue #6697

Open
wants to merge 76 commits into
base: master
Choose a base branch
from

Conversation

claravanstaden
Copy link
Contributor

@claravanstaden claravanstaden commented Nov 28, 2024

Description

Implements the Inbound Queue for Snowbridge v2.

Integration

This PR adds:

  • A new pallet called EthereumInboundQueueV2 with a submit extrinsic.
  • A new runtime API called InboundQueueApiV2 with a method dry_run, that takes an Ethereum command and converts it to Xcm and provides the execution fee on BH and static fee part of the AH execution.

Since this is a new pallet, no breaking changes are made to the EthereumInboundQueue pallet.

Review Notes

The Inbound Queue v2 pallet expects an abi-encoded envelope from Ethereum. Once decoded, the payload parameter is SCALE decoded into a v2::Message. The message contains:

  • origin: The origin address of the user on Ethereum
  • assets: A vector of assets that will be placed in the holding on AH. Can be a native ERC-20 or a foreign ERC-20 (Polkadot native assets). The XCM instructions that are used differ per asset type (ReserveAssetDeposited for native ERC-20 tokens and WithdrawAsset for Polkadot native assets). The user on Ethereum specifies additional xcm deposit the asset into a beneficiary account.
  • xcm: User provided xcms, to deposit the asset to a beneficiary, provide further fees or other instructions such as Transact (for example, to register a token)
  • claimer: The claimer on AH, in case funds are trapped so it can be claimed.

The relayer pays a DOT fee that covers:

  • The execution fees on BH
  • The static xcm part of the message on AH

The DOT fee is burnt from the relayer account and teleported to AH.

Messages may be processed out of order. Nonces are stored in a sparse bitmap for space-efficient storage.

bridges/snowbridge/pallets/inbound-queue-v2/src/lib.rs Outdated Show resolved Hide resolved
bridges/snowbridge/pallets/inbound-queue-v2/src/lib.rs Outdated Show resolved Hide resolved
bridges/snowbridge/pallets/inbound-queue-v2/src/lib.rs Outdated Show resolved Hide resolved

pub fn send_xcm(xcm: Xcm<()>, dest_para_id: u32) -> Result<XcmHash, Error<T>> {
let dest = Location::new(1, [Parachain(dest_para_id)]);
let (message_id, _) = send_xcm::<T::XcmSender>(dest, xcm).map_err(Error::<T>::from)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

You're not charging the delivery fee here, is that intended?
I know this fee should be included in the relayer reward, but that's because it's charged here, right? Else it's just more reward for the relayer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yrong @vgeddes the delivery fee is charged from the BridgeHub sovereign account, right? Do we need to deduct it from the relayer account on BH?

Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer to deduct it from the relayer account. Ideally in V2 we don't need to prefund BridgeHub sovereign account any more.

UI can estimate it off-chain with the runtime api
image

Copy link
Contributor

Choose a reason for hiding this comment

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

During v2 design I remember we found alternatives to remove all cases where v1 uses BH/AH SA accounts for covering costs.

So yes, let's charge the relayer directly. Either charge their account or better UX would be to subtract from the reward.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch Francisco. @claravanstaden this delivery fee needs to be deducted from the relayer's account.

Its the responsibility of the user initiating the bridge transfer to provide a relayer reward that covers the delivery fee.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also the delivery fee also needs to be included in the fee returned by the dry-run API

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed in 6f769cc

Delivery fee is deducted from relayers account on BH.

bridges/snowbridge/pallets/inbound-queue-v2/src/test.rs Outdated Show resolved Hide resolved
bridges/snowbridge/primitives/router/src/inbound/v2.rs Outdated Show resolved Hide resolved
@franciscoaguirre franciscoaguirre added the T15-bridges This PR/Issue is related to bridges. label Jan 27, 2025
@franciscoaguirre
Copy link
Contributor

/cmd fmt

@paritytech-review-bot paritytech-review-bot bot requested a review from a team January 27, 2025 17:59
let (xcm, _relayer_reward) =
Self::do_convert(envelope.message, origin_account_location.clone())?;

// Todo: Deposit fee(in Ether) to RewardLeger which should cover all of:
Copy link
Contributor

Choose a reason for hiding this comment

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

When will this be uncommented? Do we need to create a tracking issue?

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 reward functionality is added in #6578. Once that is merged, we can uncomment this. Added tracking issue: #7356

Copy link
Contributor

Choose a reason for hiding this comment

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

In the interim, I've added a dummy trait until the new pallet is ready: 6f769cc

@@ -0,0 +1,3 @@
# Ethereum Inbound Queue

Reads messages from Ethereum and sends it to intended destination on Polkadot, using XCM.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Reads messages from Ethereum and sends it to intended destination on Polkadot, using XCM.
Reads messages from Ethereum and sends them to intended destination on Polkadot, using XCM.

This could also use more details like architecture overview, APIs, usage patterns, etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

please also add at least some example(s) - how it can/should be used

bridges/snowbridge/pallets/inbound-queue-v2/src/api.rs Outdated Show resolved Hide resolved
Comment on lines +21 to +24
// Calculate fee. Consists of the cost of the "submit" extrinsic as well as the XCM execution
// prologue fee (static XCM part of the message that is execution on AH).
let weight_fee = T::WeightToFee::weight_to_fee(&T::WeightInfo::submit());
let fee: u128 = weight_fee.try_into().map_err(|_| Error::<T>::InvalidFee)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

I only see the submit() cost here, where is the "XCM execution prologue fee" added?

Copy link
Contributor

Choose a reason for hiding this comment

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

Turns out this runtime api implementation was incorrect. We don't need to estimate any fees here, since there are already other runtime-apis that can estimate the cost of calling submit()

Instead we actually only need a runtime api to convert the inbound message to an XCM that can be dry-run on AH.

New implemention in 6f769cc where is renamed to convert_message

Comment on lines +258 to +262
pub fn account_to_location(account: AccountIdOf<T>) -> Result<Location, Error<T>> {
let account_bytes: [u8; 32] =
account.encode().try_into().map_err(|_| Error::<T>::InvalidAccount)?;
Ok(Location::new(0, [AccountId32 { network: None, id: account_bytes }]))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

you can replace this fn with a bound on AccountIdOf<T>: Into<Location> as convertors already exist.


pub fn send_xcm(xcm: Xcm<()>, dest_para_id: u32) -> Result<XcmHash, Error<T>> {
let dest = Location::new(1, [Parachain(dest_para_id)]);
let (message_id, _) = send_xcm::<T::XcmSender>(dest, xcm).map_err(Error::<T>::from)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

During v2 design I remember we found alternatives to remove all cases where v1 uses BH/AH SA accounts for covering costs.

So yes, let's charge the relayer directly. Either charge their account or better UX would be to subtract from the reward.

bridges/snowbridge/pallets/inbound-queue-v2/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

I personally found this file confusing/obfuscating.

I would rather define pub type Nonce<T> = SparseBitmapImpl<crate::NonceBitmap<T>>; in the main inbound-queue-v2/src/lib.rs file where NonceBitmap<T> is also defined.

Seeing how this file doesn't define other types needed externally, it could then be dropped.

}
}

/// The nonce of the message been processed or not
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// The nonce of the message been processed or not
/// StorageMap used for encoding a [SparseBitmapImpl](<link-to-SparseBitmapImpl-file>) that tracks whether a specific nonce has been processed or not. Message nonces are unique and never repeated.

Comment on lines +232 to +237
// Set nonce flag to true
log::info!(target: "snowbridge-inbound-queue:v2","💫 setting nonce to {:?}", envelope.nonce);
Nonce::<T>::set(envelope.nonce.into());

// Attempt to forward XCM to AH
let message_id = Self::send_xcm(xcm, T::AssetHubParaId::get())?;
Copy link
Contributor

Choose a reason for hiding this comment

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

any error will in practice revert/rollback storage changes, but let's be paranoid and mark nonce as processed only on certainty of success:

Suggested change
// Set nonce flag to true
log::info!(target: "snowbridge-inbound-queue:v2","💫 setting nonce to {:?}", envelope.nonce);
Nonce::<T>::set(envelope.nonce.into());
// Attempt to forward XCM to AH
let message_id = Self::send_xcm(xcm, T::AssetHubParaId::get())?;
// Attempt to forward XCM to AH
let message_id = Self::send_xcm(xcm, T::AssetHubParaId::get())?;
// Set nonce flag to true
log::info!(target: "snowbridge-inbound-queue:v2","💫 setting nonce to {:?}", envelope.nonce);
Nonce::<T>::set(envelope.nonce.into());

Copy link
Contributor

Choose a reason for hiding this comment

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

fixed in 6f769cc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yrong mentioned if we increment the nonce after the send we risk re-entrancy attacks.

}

#[test]
fn test_submit_happy_path() {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe add another attempt here after the successful one and verify nonce verification fails

Copy link
Contributor

Choose a reason for hiding this comment

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

can we move these weights under mock.rs or tests.rs? I don't want them to be usable outside of tests.

Comment on lines +9 to +10
/// Burns the fees embedded in the XCM for teleports.
pub fn burn_fees<AssetTransactor, Balance>(dest: Location, fee: Balance) -> DispatchResult
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not used anywhere in this PR

you should call it in inbound_queue_v2::submit() before or within Self::send_xcm()

it could also be used in inbound-queue-v1 and you can remove old fn burn_fees()

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch, this was obsolete code. Deleted in 33f8e6d

In V2, we don't need to burn any asset for teleporting. No sovereign accounts, etc.

Copy link
Contributor

@acatangiu acatangiu Feb 3, 2025

Choose a reason for hiding this comment

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

yes, with fees paid exclusively using eth, there are no more teleports involved so no need to locally burn anything on BH

Comment on lines +9 to +10
/// Burns the fees embedded in the XCM for teleports.
pub fn burn_fees<AssetTransactor, Balance>(dest: Location, fee: Balance) -> DispatchResult
Copy link
Contributor

Choose a reason for hiding this comment

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

another note about it:

since it is defined in primitives, maybe make it less opinionated:

Suggested change
/// Burns the fees embedded in the XCM for teleports.
pub fn burn_fees<AssetTransactor, Balance>(dest: Location, fee: Balance) -> DispatchResult
/// Burns the fees embedded in the XCM for teleports.
pub fn burn_assets_for_teleport<AssetTransactor>(dest: Location, assets: Assets) -> DispatchResult

this way you can use it for various types of asset(s)

Comment on lines +45 to 46
penpal-emulated-chain = { workspace = true }
rococo-westend-system-emulated-network = { workspace = true }
Copy link
Contributor

Choose a reason for hiding this comment

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

you don't need to take direct dependency to penpal-emulated-chain here, it is already included in rococo-westend-system-emulated-network

@@ -21,11 +21,13 @@ use codec::{Decode, Encode};
use emulated_integration_tests_common::{PENPAL_B_ID, RESERVABLE_ASSET_ID};
use frame_support::pallet_prelude::TypeInfo;
use hex_literal::hex;
use penpal_emulated_chain::PARA_ID_B;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
use penpal_emulated_chain::PARA_ID_B;
use rococo_westend_system_emulated_network::penpal_emulated_chain::PARA_ID_B;

Comment on lines +223 to +232

pub fn exchange_asset() -> Weight {
// Proof Size summary in bytes:
// Measured: `159`
// Estimated: `6196`
// Minimum execution time: 87_253_000 picoseconds.
Weight::from_parts(88_932_000, 6196)
.saturating_add(T::DbWeight::get().reads(9))
.saturating_add(T::DbWeight::get().writes(4))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be benchmarked eventually, but it's fine like this for Westend - it will get benchmarked outside of this PR at a later point

@@ -71,7 +74,9 @@ parameter_types! {
};
pub AssetHubFromEthereum: Location = Location::new(1,[GlobalConsensus(RelayNetwork::get()),Parachain(westend_runtime_constants::system_parachain::ASSET_HUB_ID)]);
pub EthereumUniversalLocation: InteriorLocation = [GlobalConsensus(EthereumNetwork::get())].into();
pub WethAddress: H160 = H160(hex_literal::hex!("fff9976782d46cc05630d1f6ebab18b2324d6b14"));
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not used anywhere

Suggested change
pub WethAddress: H160 = H160(hex_literal::hex!("fff9976782d46cc05630d1f6ebab18b2324d6b14"));

@vgeddes vgeddes mentioned this pull request Jan 30, 2025
Comment on lines +33 to +34
/// The full value of the assets.
pub value: u128,
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add more info here? what is the full value of the assets?

There's an assets field above that can hold arbitrary number of different tokens with arbitrary amounts of each.
Is that field related to this value: u128 field? If yes, how? What is the relationship between them? If not, then what is this value?

pseudo-random guess: does it represent an estimate of the eth value of all assets above?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, that comment is very incorrect - I should have picked it up in my reviews.

message.value is the amount of native ether that was bridged from Ethereum.

message.assets holds other kind of assets, ERC20, NFTs, etc.

Comment on lines +131 to +132
let xcm_string = hex::encode(message.xcm.clone());
log::info!(target: LOG_TARGET,"found xcm payload: {:x?}", xcm_string);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let xcm_string = hex::encode(message.xcm.clone());
log::info!(target: LOG_TARGET,"found xcm payload: {:x?}", xcm_string);

Comment on lines +135 to +146
if let Ok(versioned_xcm) = VersionedXcm::<()>::decode_with_depth_limit(
MAX_XCM_DECODE_DEPTH,
&mut message.xcm.as_ref(),
) {
if let Ok(decoded_xcm) = versioned_xcm.try_into() {
message_xcm = decoded_xcm;
} else {
log::error!(target: LOG_TARGET,"unable to decode xcm");
}
} else {
log::error!(target: LOG_TARGET,"unable to decode versioned xcm");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

failing this should fail the whole conversion, no?

short-form suggestion:

Suggested change
if let Ok(versioned_xcm) = VersionedXcm::<()>::decode_with_depth_limit(
MAX_XCM_DECODE_DEPTH,
&mut message.xcm.as_ref(),
) {
if let Ok(decoded_xcm) = versioned_xcm.try_into() {
message_xcm = decoded_xcm;
} else {
log::error!(target: LOG_TARGET,"unable to decode xcm");
}
} else {
log::error!(target: LOG_TARGET,"unable to decode versioned xcm");
}
if let Ok(decoded_xcm) = VersionedXcm::<()>::decode_with_depth_limit(
MAX_XCM_DECODE_DEPTH,
&mut message.xcm.as_ref(),
).map(|versioned_xcm| versioned_xcm.try_into()) {
message_xcm = decoded_xcm;
} else {
let xcm_string = hex::encode(message.xcm);
log::debug!(target: LOG_TARGET, "unable to decode xcm: {:x?}", xcm_string);
return Err(ConvertMessageError::InvalidXcm);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@acatangiu should we not rather fail silent on invalid xcm, so that assets can be trapped on AH? Otherwise the funds will be locked on Ethereum but not claimable on Polkadot. This was @alistair-singh 's idea.

}
}

log::info!(target: LOG_TARGET,"xcm decoded as {:?}", message_xcm);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
log::info!(target: LOG_TARGET,"xcm decoded as {:?}", message_xcm);
log::trace!(target: LOG_TARGET,"xcm decoded as {:?}", message_xcm);

let network = EthereumNetwork::get();

// use eth as asset
let fee_asset = Location::new(2, [GlobalConsensus(EthereumNetwork::get())]);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
let fee_asset = Location::new(2, [GlobalConsensus(EthereumNetwork::get())]);
let fee_asset_id = Location::new(2, [GlobalConsensus(EthereumNetwork::get())]);

let eth: XcmAsset =
(fee_asset.clone(), message.execution_fee.saturating_add(message.value)).into();
let mut instructions = vec![
DescendOrigin(PalletInstance(InboundQueuePalletInstance::get()).into()),
Copy link
Contributor

Choose a reason for hiding this comment

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

rather than InboundQueuePalletInstance: Get<u8> as generic on this fn, I would use the full location InboundQueueLocation: Get<InteriorLocation> and here:

Suggested change
DescendOrigin(PalletInstance(InboundQueuePalletInstance::get()).into()),
DescendOrigin(InboundQueueLocation::get()),

Comment on lines +170 to +171
// If the claimer can be decoded, add it to the message. If the claimer decoding fails,
// do not add it to the message, because it will cause the xcm to fail.
Copy link
Contributor

Choose a reason for hiding this comment

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

yes, not clear if we should fail the whole thing or just continue with default claimer...

Copy link
Contributor

Choose a reason for hiding this comment

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

I have an idea of a more general solution for any type of failures on inbound queue that are caused by protocol incompatibilities (bridge's fault, not the user's):

the inbound queue verifies the authenticity and exact validity of the message through the prover, so if it fails on some decoding step we can consider the message invalid, it will always fail, and hopefully we can revert it the Ethereum side.
So maybe we can mark it in some other InvalidMessage: SparseBitmap storage item or simply emit an InvalidMessage(nonce) event that can be proved back to Ethereum so that it can be rolled back there in case of these kinds of bridge protocol issues.

This is a further enhancement idea to be done outside of this PR, but worth exploring.

Copy link
Contributor

Choose a reason for hiding this comment

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

This makes sense 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same comment as here: #6697 (comment) It might be better to allow the invalid claimer, so that the funds can be trapped on AH.

The future improvement sounds cool!

Comment on lines +172 to +173
if let Ok(claimer) = Junction::decode(&mut claimer.as_ref()) {
let claimer_location: Location = Location::new(0, [claimer.into()]);
Copy link
Contributor

Choose a reason for hiding this comment

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

why is the claimer just a Junction / InteriorLocation? seems like a gratuitous limitation. You should leave it as full Location specifiable on Ethereum side.

Could be a local account (just a Junction like now) or it could be a remote location that will be derived into a sov acc.

refund_surplus_to = claimer_location.clone();
instructions.push(SetHints {
hints: vec![AssetClaimer { location: claimer_location }].try_into().unwrap(),
}); // TODO
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO what? :)

Comment on lines +226 to +235
let appendix = vec![
RefundSurplus,
// Refund excess fees to the claimer, if present, otherwise to the relayer.
DepositAsset {
assets: Wild(AllOf { id: AssetId(fee_asset.into()), fun: WildFungible }),
beneficiary: refund_surplus_to,
},
];

instructions.extend(appendix);
Copy link
Contributor

Choose a reason for hiding this comment

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

using instructions.push() for each of the appendix elements would avoid allocating another intermediary appendix vec

pub const EthereumNetwork: xcm::v5::NetworkId = xcm::v5::NetworkId::Ethereum { chain_id: 11155111 };
pub const GatewayAddress: H160 = H160(GATEWAY_ADDRESS);
pub const InboundQueuePalletInstance: u8 = 84;
pub AssetHubLocation: InteriorLocation = Parachain(1000).into();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pub AssetHubLocation: InteriorLocation = Parachain(1000).into();

Comment on lines +302 to +303
let instructions = vec![
DepositAsset { assets: Wild(AllCounted(1).into()), beneficiary },
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let instructions = vec![
DepositAsset { assets: Wild(AllCounted(1).into()), beneficiary },
let instructions = vec![
RefundSurplus,
DepositAsset { assets: Wild(AllCounted(1).into()), beneficiary },

let mut descend_origin_found = 0;
let mut reserve_deposited_found = 0;
let mut withdraw_assets_found = 0;
while let Some(instruction) = instructions.next() {
Copy link
Contributor

Choose a reason for hiding this comment

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

nice test! please also add checks for the inner xcm instructions being included

assert_ok!(<AssetHubWestend as AssetHubWestendPallet>::ForeignAssets::force_create(
RuntimeOrigin::root(),
token_location.clone().try_into().unwrap(),
assethub_sovereign.clone().into(),
Copy link
Contributor

Choose a reason for hiding this comment

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

why use this account as the token admin?

this account is AH's sov acc as derived on BH (has meaning of SA in BH context); then this is used as local account on AH (account has no meaning on AH, it's just a "random account").

});
}

pub(crate) fn set_up_eth_and_dot_pool(asset: v5::Location) {
Copy link
Contributor

Choose a reason for hiding this comment

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

you can drop this function and use this existing one instead

assert_expected_events!(
AssetHubWestend,
vec![
RuntimeEvent::ForeignAssets(pallet_assets::Event::Issued { .. }) => {},
Copy link
Contributor

Choose a reason for hiding this comment

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

check asset id - also check for all assets

example here how to use this macro to also check inner values

// Check that the token was received and issued as a foreign asset on PenpalB
assert_expected_events!(
PenpalB,
vec![RuntimeEvent::ForeignAssets(pallet_assets::Event::Issued { .. }) => {},]
Copy link
Contributor

Choose a reason for hiding this comment

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

check for all assets including their IDs

let asset_id: Location =
[PalletInstance(ASSETS_PALLET_ID), GeneralIndex(RESERVABLE_ASSET_ID.into())].into();

register_foreign_asset(eth_location());
Copy link
Contributor

Choose a reason for hiding this comment

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

you should just make ETH (and maybe WETH) part of AH genesis for these tests and not have to register for every test: e.g.


BridgeHubWestend::execute_with(|| {
type RuntimeEvent = <BridgeHubWestend as Chain>::RuntimeEvent;
let instructions = vec![DepositAsset { assets: Wild(AllCounted(2)), beneficiary }];
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let instructions = vec![DepositAsset { assets: Wild(AllCounted(2)), beneficiary }];
let instructions = vec![RefundSurplus, DepositAsset { assets: Wild(AllCounted(2)), beneficiary }];

events.iter().any(|event| matches!(
event,
RuntimeEvent::Assets(pallet_assets::Event::Burned { owner, .. })
if *owner == ethereum_sovereign.clone(),
Copy link
Contributor

Choose a reason for hiding this comment

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

why is it burned from ethereum_sovereign?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yrong correct me if I am wrong, but Polkadot native assets are locked in the Ethereum sovereign account, and when sent back to Ethereum it is then burnt.

@paritytech-review-bot paritytech-review-bot bot requested a review from a team February 3, 2025 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T15-bridges This PR/Issue is related to bridges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants