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: deposit extra fee to beneficiary on Asset Hub #4175

Merged
merged 16 commits into from
Apr 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 4 additions & 2 deletions bridges/snowbridge/primitives/router/src/inbound/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -273,8 +273,10 @@ where
},
None => {
instructions.extend(vec![
// Deposit asset to beneficiary.
DepositAsset { assets: Definite(asset.into()), beneficiary },
// Deposit both asset and fees to beneficiary so the fees will not get
// trapped. Another benefit is when fees left more than ED on AssetHub could be
// used to create the beneficiary account in case it does not exist.
DepositAsset { assets: Wild(AllCounted(2)), beneficiary },
Copy link
Contributor

@bkontur bkontur Apr 25, 2024

Choose a reason for hiding this comment

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

You could maybe add description before every instruction for let mut instructions = vec![ :)

One question: could you point me to the place or explain where asset_hub_fee and destination.fee is coming from? Is it withdrawn or burned somewhere on BH? Is it the user's money, or is it money from some sovereign account on BH? Or where does this money come from?

I think it can be specific (not sure if compiles):

Suggested change
DepositAsset { assets: Wild(AllCounted(2)), beneficiary },
DepositAsset {
assets: Definite(vec![asset, asset_hub_fee_asset].into())),
beneficiary
},

or maybe use total_fee_asset(dest_para_fee should be 0 in this branch) instead of asset_hub_fee_asset?

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't you need to fix also DepositReserveAsset { branch?

// Deposit asset to beneficiary.
DepositAsset { assets: Definite(asset.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.

One question: could you point me to the place or explain where asset_hub_fee and destination.fee is coming from? Is it withdrawn or burned somewhere on BH? Is it the user's money, or is it money from some sovereign account on BH? Or where does this money come from?

My concern here is whether we should deposit unused fees directly to the beneficiary instead of depositing them into a sovereign account in the AH. If this is a valid scenario, then the process should resemble something like this:

DepositAsset { assets: Definite(vec![asset].into())), beneficiary},
DepositAsset { assets: Definite(vec![asset_hub_fee_asset].into())), beneficiary: LocationOfSomeSovereignAccountOnAssetHub},

Copy link
Contributor Author

Choose a reason for hiding this comment

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

where asset_hub_fee and destination.fee is coming from?

asset_hub_fee from https://github.com/Snowfork/snowbridge/blob/3f90b1619d0bdbec8f50b63185b3dcc632321019/contracts/src/storage/AssetsStorage.sol#L15 which is a configurable storage on Ethereum, we charge it from the user and make sure that it can cover the execution cost on AH.

destination.fee is always zero for AH.

whether we should deposit unused fees directly to the beneficiary

I think so, it's from end user and we do want to send the unused back to the user.

Don't you need to fix also DepositReserveAsset { branch?

Nope, that branch is for non-system parachain which may not accept AssetHub-DOT as fee asset, we're gonna to make some fix there later and this PR focus on AH only.

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 so, it's from end user and we do want to send the unused back to the user.

ok, cool, thank you :)

Don't you need to fix also DepositReserveAsset { branch?

Nope, that branch is for non-system parachain which may not accept AssetHub-DOT as fee asset, we're gonna to make some fix there later and this PR focus on AH only.

Ok, got it. I saw that you use Location::parent() here for fees.assetId, so that would be probably changed and/or Destination.fee: u128, let's see later.

So, I think using exact assets for DepositAsset, as suggested above, is not a bad idea (or maybe some exact counter of unique assetIds :))

]);
},
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ use snowbridge_pallet_inbound_queue_fixtures::{
};
use snowbridge_pallet_system;
use snowbridge_router_primitives::inbound::{
Command, GlobalConsensusEthereumConvertsFor, MessageV1, VersionedMessage,
Command, Destination, GlobalConsensusEthereumConvertsFor, MessageV1, VersionedMessage,
};
use sp_core::H256;
use sp_runtime::{DispatchError::Token, TokenError::FundsUnavailable};
Expand All @@ -40,6 +40,7 @@ const TREASURY_ACCOUNT: [u8; 32] =
const WETH: [u8; 20] = hex!("87d1f7fdfEe7f651FaBc8bFCB6E086C278b77A7d");
const ETHEREUM_DESTINATION_ADDRESS: [u8; 20] = hex!("44a57ee2f2FCcb85FDa2B0B18EBD0D8D2333700e");
const INSUFFICIENT_XCM_FEE: u128 = 1000;
const XCM_FEE: u128 = 4_000_000_000;

#[derive(Encode, Decode, Debug, PartialEq, Eq, Clone, TypeInfo)]
pub enum ControlCall {
Expand Down Expand Up @@ -555,3 +556,133 @@ fn register_weth_token_in_asset_hub_fail_for_insufficient_fee() {
);
});
}

fn send_token_from_ethereum_to_asset_hub_with_fee(account_id: [u8; 32], fee: u128) {
let weth_asset_location: Location = Location::new(
2,
[EthereumNetwork::get().into(), AccountKey20 { network: None, key: WETH }],
);
// (Parent, Parent, EthereumNetwork::get(), AccountKey20 { network: None, key: WETH })
// Fund asset hub sovereign on bridge hub
let asset_hub_sovereign = BridgeHubRococo::sovereign_account_id_of(Location::new(
1,
[Parachain(AssetHubRococo::para_id().into())],
));
BridgeHubRococo::fund_accounts(vec![(asset_hub_sovereign.clone(), INITIAL_FUND)]);

// Register WETH
AssetHubRococo::execute_with(|| {
type RuntimeOrigin = <AssetHubRococo as Chain>::RuntimeOrigin;

assert_ok!(<AssetHubRococo as AssetHubRococoPallet>::ForeignAssets::force_create(
RuntimeOrigin::root(),
weth_asset_location.clone().try_into().unwrap(),
asset_hub_sovereign.into(),
false,
1,
));

assert!(<AssetHubRococo as AssetHubRococoPallet>::ForeignAssets::asset_exists(
weth_asset_location.clone().try_into().unwrap(),
));
});

// Send WETH to an existent account on asset hub
BridgeHubRococo::execute_with(|| {
type RuntimeEvent = <BridgeHubRococo as Chain>::RuntimeEvent;

type EthereumInboundQueue =
<BridgeHubRococo as BridgeHubRococoPallet>::EthereumInboundQueue;
let message_id: H256 = [0; 32].into();
let message = VersionedMessage::V1(MessageV1 {
chain_id: CHAIN_ID,
command: Command::SendToken {
token: WETH.into(),
destination: Destination::AccountId32 { id: account_id },
amount: 1_000_000,
fee,
},
});
let (xcm, _) = EthereumInboundQueue::do_convert(message_id, message).unwrap();
assert_ok!(EthereumInboundQueue::send_xcm(xcm, AssetHubRococo::para_id().into()));

// Check that the message was sent
assert_expected_events!(
BridgeHubRococo,
vec![
RuntimeEvent::XcmpQueue(cumulus_pallet_xcmp_queue::Event::XcmpMessageSent { .. }) => {},
]
);
});
}

#[test]
fn send_token_from_ethereum_to_existent_account_on_asset_hub() {
send_token_from_ethereum_to_asset_hub_with_fee(AssetHubRococoSender::get().into(), XCM_FEE);

AssetHubRococo::execute_with(|| {
type RuntimeEvent = <AssetHubRococo as Chain>::RuntimeEvent;

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

#[test]
fn send_token_from_ethereum_to_non_existent_account_on_asset_hub() {
send_token_from_ethereum_to_asset_hub_with_fee([1; 32], XCM_FEE);

AssetHubRococo::execute_with(|| {
type RuntimeEvent = <AssetHubRococo as Chain>::RuntimeEvent;

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

#[test]
fn send_token_from_ethereum_to_non_existent_account_on_asset_hub_with_insufficient_fee() {
send_token_from_ethereum_to_asset_hub_with_fee([1; 32], INSUFFICIENT_XCM_FEE);

AssetHubRococo::execute_with(|| {
type RuntimeEvent = <AssetHubRococo as Chain>::RuntimeEvent;

// Check that the message was not processed successfully due to insufficient fee

assert_expected_events!(
AssetHubRococo,
vec![
RuntimeEvent::MessageQueue(pallet_message_queue::Event::Processed { success:false, .. }) => {},
]
);
});
}

#[test]
fn send_token_from_ethereum_to_non_existent_account_on_asset_hub_with_sufficient_fee_but_do_not_satisfy_ed(
) {
// On AH the xcm fee is 33_873_024 and the ED is 3_300_000
send_token_from_ethereum_to_asset_hub_with_fee([1; 32], 36_000_000);

AssetHubRococo::execute_with(|| {
type RuntimeEvent = <AssetHubRococo as Chain>::RuntimeEvent;

// Check that the message was not processed successfully due to insufficient ED
assert_expected_events!(
AssetHubRococo,
vec![
RuntimeEvent::MessageQueue(pallet_message_queue::Event::Processed { success:false, .. }) => {},
]
);
});
}
13 changes: 13 additions & 0 deletions prdoc/pr_4175.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0
# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json

title: "Snowbridge: deposit extra fee to beneficiary on Asset Hub"

doc:
- audience: Runtime Dev
description: |
Snowbridge transfers arriving on Asset Hub will deposit both asset and fees to beneficiary so the fees will not get trapped.
Another benefit is when fees left more than ED, could be used to create the beneficiary account in case it does not exist on asset hub.

crates:
- name: snowbridge-router-primitives
Loading