Skip to content
This repository has been archived by the owner on Oct 22, 2024. It is now read-only.

Revamp xcm for the reserver tranfer from substrate to Ethreum & Implement a custom exporter #160

Closed
wants to merge 7 commits into from
Closed

Revamp xcm for the reserver tranfer from substrate to Ethreum & Implement a custom exporter #160

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d489756
Focus on the xcm construction & Remove reward
yrong Jul 23, 2024
6729e4b
Change the value of local fee
yrong Jul 23, 2024
046c9fb
Merge branch 'snowbridge' into ron/custom-execute-transfer
yrong Jul 29, 2024
e927ced
Improve test checking balance
yrong Jul 29, 2024
93f952e
Adjust fees more accurate
yrong Jul 30, 2024
88c31b1
Add a custom SovereignReceiveTeleportRemoteExporter
yrong Jul 30, 2024
a5a384c
Revamp the xcm
yrong Jul 31, 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
28 changes: 17 additions & 11 deletions ...chains/integration-tests/emulated/tests/bridges/bridge-hub-rococo/src/tests/snowbridge.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,19 +381,15 @@ fn send_weth_asset_from_asset_hub_to_ethereum() {
let assethub_sovereign = BridgeHubRococo::sovereign_account_id_of(assethub_location);
let bridgehub_location = AssetHubRococo::sibling_location_of(BridgeHubRococo::para_id());

AssetHubRococo::force_default_xcm_version(Some(XCM_VERSION));
BridgeHubRococo::force_default_xcm_version(Some(XCM_VERSION));
AssetHubRococo::force_xcm_version(
Location::new(2, [GlobalConsensus(Ethereum { chain_id: CHAIN_ID })]),
XCM_VERSION,
);

BridgeHubRococo::fund_accounts(vec![(assethub_sovereign.clone(), INITIAL_FUND)]);

Copy link
Author

Choose a reason for hiding this comment

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

Should remove this initial fund as there is no need to fund the sovereign here any more.

const WETH_AMOUNT: u128 = 1_000_000_000;
const FEE_AMOUNT: u128 = 2_750_872_500_000;
// To cover the delivery cost on BH
const LOCAL_FEE_AMOUNT: u128 = 1_000_000_000;
const LOCAL_FEE_AMOUNT: u128 = DefaultBridgeHubEthereumBaseFee::get() + 12_000_000;
// To cover the delivery cost on Ethereum
const REMOTE_FEE_AMOUNT: u128 = FEE_AMOUNT - LOCAL_FEE_AMOUNT;

Expand Down Expand Up @@ -457,29 +453,36 @@ fn send_weth_asset_from_asset_hub_to_ethereum() {
BuyExecution { fees: local_fee_asset.clone(), weight_limit: Unlimited },
DepositAsset {
assets: Wild(AllCounted(1)),
beneficiary: (AccountId32 { id: assethub_sovereign.into(), network: None },).into(),
beneficiary:
(AccountId32 { id: assethub_sovereign.clone().into(), network: None },).into(),
},
]);

let xcms = VersionedXcm::from(Xcm(vec![
WithdrawAsset(assets.clone().into()),
SetFeesMode { jit_withdraw: true },
Copy link

Choose a reason for hiding this comment

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

Why SetFeesMode instead of BuyExecution? I think the user should have an opportunity to pass the necessary fee amounts after estimating fees using the XCM dry run extrinsic.

Also they want to deprecate SetFeesMode in polkadot-fellows/xcm-format#57.

Copy link
Author

@yrong yrong Jul 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The fee asset(DOT) is already included in WithdrawAsset(assets.clone().into()), actually we transfer 2 kinds of assets to Ethereum(DOT and WETH) and use DOT to pay for the delivery cost on BH.

The SetFeesMode is taking the delivery cost on AH from user for InitiateReserveWithdraw, suggest to run the test case with

RUST_LOG=xcm=trace cargo test -p bridge-hub-rococo-integration-tests --lib tests::snowbridge::send_weth_asset_from_asset_hub_to_ethereum -- --nocapture

and check logs as follows for detail:

...
2024-07-23T02:04:58.003798Z TRACE xcm::fees: taking fee: Assets([Asset { id: AssetId(Location { parents: 1, interior: Here }), fun: Fungible(1106266603) }]) from origin_ref: Some(Location { parents: 0, interior: X1([AccountId32 { network: Some(Rococo), id: [142, 175, 4, 21, 22, 135, 115, 99, 38, 201, 254, 161, 126, 37, 252, 82, 135, 97, 54, 147, 201, 18, 144, 156, 178, 38, 170, 71, 148, 242, 106, 72] }]) }) in fees_mode: FeesMode { jit_withdraw: true } for a reason: InitiateReserveWithdraw

Copy link

@vgeddes vgeddes Jul 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Can't the executor take fees from the holding register? DOT has already been transferred to the holding register after the execution of WithdrawAsset.

Copy link
Author

Choose a reason for hiding this comment

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

We can but I don't see benefit for adding an extra BuyExecution if SetFeesMode can work here.

Btw: polkadot-fellows/xcm-format#57 is closed so it won't impact and I expect more on polkadot-fellows/RFCs#105 which is another story.

InitiateTeleport {
assets: Definite(vec![local_fee_asset.clone()].into()),
xcm: teleport_xcm_on_bh,
dest: bridgehub_location,
},
InitiateReserveWithdraw {
Copy link

@vgeddes vgeddes Jul 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

OK, this looks good, fairly simple 👍

Can you check what we would need for Polkadot-native assets? I suspect its TransferReserveAsset with the destination being our gateway contract.

Copy link
Author

@yrong yrong Jul 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I suspect its TransferReserveAsset

Exactly.

destination being our gateway contract.

Currently the destination is always (Location::new(2, [GlobalConsensus(Ethereum { chain_id: CHAIN_ID })])

yrong marked this conversation as resolved.
Show resolved Hide resolved
assets: Definite(vec![remote_fee_asset.clone(), weth_asset.clone()].into()),
// with reserve set to Ethereum destination, the ExportMessage will
// be appended to the front of the list by the SovereignPaidRemoteExporter
reserve: destination,
xcm: withdraw_xcm_on_bh,
},
InitiateTeleport {
assets: Definite(vec![local_fee_asset.clone()].into()),
xcm: teleport_xcm_on_bh,
dest: bridgehub_location,
},
]));
let free_balance_before = <AssetHubRococo as AssetHubRococoPallet>::Balances::free_balance(
AssetHubRococoReceiver::get(),
);
// Assert there is no balance left in the assethub_sovereign on BH
let free_balance_of_sovereign_on_bh_before =
<BridgeHubRococo as BridgeHubRococoPallet>::Balances::free_balance(
assethub_sovereign.clone(),
);
assert_eq!(free_balance_of_sovereign_on_bh_before, 0);
<AssetHubRococo as AssetHubRococoPallet>::PolkadotXcm::execute(
RuntimeOrigin::signed(AssetHubRococoReceiver::get()),
bx!(xcms),
Expand All @@ -504,6 +507,9 @@ fn send_weth_asset_from_asset_hub_to_ethereum() {
RuntimeEvent::EthereumOutboundQueue(snowbridge_pallet_outbound_queue::Event::MessageQueued {..}) => {},
]
);
let free_balance_of_sovereign_on_bh_after =
<BridgeHubRococo as BridgeHubRococoPallet>::Balances::free_balance(assethub_sovereign);
assert_eq!(free_balance_of_sovereign_on_bh_after, 955613334);
Copy link
Author

@yrong yrong Jul 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Currently there is no need to fund the sovereign account on BH any more as we will use a InitiateTeleport instruction for an instant top up.

As we may notice after the test there is still some fee left unused in the sov account. So bad guys can leverage that to ignore the InitiateTeleport instruction later.

Another issue for current impl is that we have to charge twice from user for the DefaultBridgeHubEthereumBaseFee, one for the InitiateTeleport for an instant top up as mentioned before, the other for the delivery cost here sending the remote xcm of InitiateReserveWithdraw to Ethreum through BH for the Export.

Can run RUST_LOG=xcm=trace cargo test -p bridge-hub-rococo-integration-tests --lib tests::snowbridge::send_weth_asset_from_asset_hub_to_ethereum -- --nocapture for the execution path.

Copy link
Author

Choose a reason for hiding this comment

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

Addressed in #167

});
}

Expand Down
Loading