-
Notifications
You must be signed in to change notification settings - Fork 739
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
Changes from 5 commits
8f86f35
0a218c7
3fd810c
db52103
bf3856b
908442b
54d8d4e
dee6ecc
76755f9
3eed3aa
bd36b4d
45481f9
3c7a66d
3c1ac1a
a194ca5
b79bc16
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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}; | ||
|
@@ -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 { | ||
|
@@ -555,3 +556,132 @@ 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 token was received and issued as a foreign asset on AssetHub | ||
yrong marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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 token was received and issued as a foreign asset on AssetHub | ||
yrong marked this conversation as resolved.
Show resolved
Hide resolved
|
||
assert_expected_events!( | ||
AssetHubRococo, | ||
vec![ | ||
RuntimeEvent::MessageQueue(pallet_message_queue::Event::Processed { success:false, .. }) => {}, | ||
] | ||
); | ||
}); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so it looks like it does fail, losing the ETH as well - was better off before when extra fees would just get trapped - at least then ETH always went through I think you should do a Later, when we have #4190 in XCMv5, we can simplify and just trap "unhandled" assets with no worries as they can be easily claimed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, we do need the @acatangiu In this commit I do add As you can see from the log the test So I'm not sure if it provides benefit here.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, that's right, it doesn't help since account doesn't exist and WETH can't count towards ED.. I guess we'll just have to trap for now |
There was a problem hiding this comment.
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
anddestination.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):
or maybe use
total_fee_asset(dest_para_fee should be 0 in this branch)
instead ofasset_hub_fee_asset
?There was a problem hiding this comment.
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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.I think so, it's from end user and we do want to send the unused back to the user.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, cool, thank you :)
Ok, got it. I saw that you use
Location::parent()
here for fees.assetId, so that would be probably changed and/orDestination.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 :))