Adds transfer token user fee #7

claravanstaden · 2023-11-01T09:57:24Z

Adds user fee for transfer token on asset hub.

Snowbridge companion: Snowfork/snowbridge#987
Resolves: SNO-732

claravanstaden · 2023-11-01T11:37:23Z

bridges/primitives/chain-asset-hub-rococo/src/lib.rs

+
+	/// User fee for ERC20 token transfer back to Ethereum.
+	/// (initially was calculated by test `OutboundQueue::calculate_fees` - ETH/ROC 1/411 and fee_per_gas 15 GWEI = 18679250000 + *25%)
+	pub const TransferERC20TokenBaseFeeInRocs: u128 = 23349062500;


Not sure this is the best naming, I'm open to suggestions.

I would suggest BridgeHubEthereumBaseFeeInRocs to be consistent with the Rocococ<->Wococo bridge.

vgeddes · 2023-11-01T19:11:36Z

bridges/primitives/chain-asset-hub-rococo/src/lib.rs

I'm not sure I would put our bridge stuff into crates in the bridges project. Its very specific to the polkadot-kusama bridge.

I would personally put this constant in a file like snowbridge_config.rs under /cumulus/parachains/runtimes/assets/asset-hub/rococo/

Then if we end up adding a whole more config, we can create a crate for it.

We could also add other constants to the module, like EthereumGatewayAddress to the module

vgeddes · 2023-11-01T19:13:57Z

bridges/primitives/chain-asset-hub-rococo/src/lib.rs

+
+	/// User fee for ERC20 token transfer back to Ethereum.
+	/// (initially was calculated by test `OutboundQueue::calculate_fees` - ETH/ROC 1/411 and fee_per_gas 15 GWEI = 18679250000 + *25%)
+	pub const TransferERC20TokenBaseFeeInRocs: u128 = 23349062500;


I would suggest BridgeHubEthereumBaseFeeInRocs to be consistent with the Rocococ<->Wococo bridge.

vgeddes · 2023-11-01T19:20:53Z

bridges/primitives/chain-asset-hub-rococo/src/lib.rs

@@ -46,6 +46,10 @@ frame_support::parameter_types! {
 	/// Base delivery fee to `BridgeHubRococo`.
 	/// (initially was calculated by test `BridgeHubRococo::can_calculate_weight_for_paid_export_message_with_reserve_transfer`)
 	pub const BridgeHubRococoBaseFeeInRocs: u128 = 1214739988;
+
+	/// User fee for ERC20 token transfer back to Ethereum.
+	/// (initially was calculated by test `OutboundQueue::calculate_fees` - ETH/ROC 1/411 and fee_per_gas 15 GWEI = 18679250000 + *25%)


The fee parameters should result in a fee that is greater or equal to the fee generated by these parameters:

https://github.com/Snowfork/snowbridge/blob/2be6f57a2bc2c0e162cda32c331dafe5b80a8b80/parachain/pallets/outbound-queue/src/lib.rs#L252

So its a problem if fee_per_gas is 15gwei here and 30gwei on bridgehub. As it will cause messaging to fail on our E2E environment

I didn't realize those values in https://github.com/Snowfork/snowbridge/blob/2be6f57a2bc2c0e162cda32c331dafe5b80a8b80/parachain/pallets/outbound-queue/src/lib.rs#L252 are the final values I should use. I wrote a script that calculates the average gas price for the last three weeks like you suggested, and it was around 15 gwei. I'll change the calculation to use the values defined in the outbound queue then.

Ah, rather update the values in the outbound queue, as your figure is more accurate :)

To clarify, I wasn't saying those values were final, but rather that if we update the fees to be more accurate in one place, we must reconcile the fees in other places too.

Got it, agreed. Will add a comment about it too.

claravanstaden · 2023-11-02T15:22:14Z

cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/xcm_config.rs

@@ -226,9 +226,9 @@ impl Contains<RuntimeCall> for SafeCallFilter {
 					snowbridge_ethereum_beacon_client::Call::force_checkpoint { .. } |
 						snowbridge_ethereum_beacon_client::Call::set_operating_mode { .. },
 				) | RuntimeCall::EthereumInboundQueue(
-					snowbridge_inbound_queue::Call::set_operating_mode { .. },
+				snowbridge_inbound_queue::Call::set_operating_mode { .. },


* add test for asset-reserve and fee-reserve both at destination * add test for asset-reserve and fee-reserve both at remote chain * add test for asset-reserve at destination while fee-reserve is local * add test for asset local-reserve while fee-reserve is destination * add test for asset local-reserve while fee-reserve is remote chain * refactor tests using better naming and conceptual examples * add test for asset destination-reserve while fee-reserve is remote chain * fix some typos * deduplicate code for do_teleport_assets() * add test for asset local-reserve while teleporting fees * add test for asset destination-reserve while teleporting fees * fix cases when asset reserve is remote, add test remote-asset and teleported fee * add test for transfer asset remote reserve and fee local reserve * add test for transfer asset remote reserve and fee destination reserve * disallow teleportable assets in reserve-transfer, add regression test * asset-hubs: fix emulated tests and deduplicate code * asset-hubs: use non-system para IDs in tests where non-system paras are intended * pallet-xcm: refactor newly added tests * pallet-xcm: fix benchmarks * fix pallet-xcm benchmarks for all runtimes * address review comments * expose TransferType through XcmExecutor::traits::AssetTransferSupport instead of pallet_xcm * allow transfer for non-fungible assets too * fix merge damage * fmt * pallet-xcm: split asset transfer tests to own file * address review comments * pallet-xcm: disallow combining remote reserves with other xfer types * xcm-barriers: allow SetFeesMode in BuyExecution barrier and fix tests * adds message queue pallet * adds outbound queue * fix compiler errors * adds snowbridge to rococo bridgehub * asset hub rococo * asset hub rococo * finishing up applying changes * fix incorrect pallet-xcm imports * verify assets and beneficiary in over-bridge test * fix AHs tests * AHs: fix emulated tests * AHs: include delivery-fee checking in tests * fix runtime-benchmarks for AHRococo * fix clippy * fixing tests * adds upstream changes * cleanup comments, fix upgrade gateway test * withdraw fees before buyexecution - still broken because of executor appended ClearOrigin * pallet-xcm: uses single custom XCM to send both fees and assets * pallet-xcm: handle teleport checking account when custom burn+teleport * fixes plus tests * fix pallet-xcm tests * fix AHs tests * attempts to fix xcm config * fixes send token * remove unused SetFeesMode instruction * add missing import * allows all networks * fmt * pallet-xcm: fix broken reserve_transfer_assets benchmark * try add pallet-assets for benchmarking to rococo * Revert "try add pallet-assets for benchmarking to rococo" This reverts commit c82330b. * pallet-xcm benchmarking: most chains do not have pallet-assets, use pallet-balances instead * pallet-xcm: fix teleport_assets benchmark * fix runtimes benchmarks for pallet-xcm * AHs simplify test_cases_over_bridge * runtimes: add dedicated benchmarking config for pallet-xcm * AHs benchmarks: fix transfer to sibling parachain * fixed token transfer test * fixes after rebase * fixes after rebase * fixes after rebase * Update polkadot/xcm/xcm-executor/src/traits/asset_transfer.rs Co-authored-by: Francisco Aguirre <[email protected]> * Update polkadot/xcm/pallet-xcm/src/lib.rs Co-authored-by: Francisco Aguirre <[email protected]> * fmt * Polkadot sdk update v2 (#3) * adds message queue pallet * adds outbound queue * fix compiler errors * adds snowbridge to rococo bridgehub * asset hub rococo * asset hub rococo * finishing up applying changes * fix incorrect pallet-xcm imports * fixing tests * adds upstream changes * cleanup comments, fix upgrade gateway test * attempts to fix xcm config * fixes send token * fmt * fixes after rebase * fixes after rebase * correct relay network check --------- Co-authored-by: claravanstaden <Cats 4 life!> * Runtime changes for refactored outbound-queue pallet * updates relay * Update polkadot/xcm/xcm-builder/src/barriers.rs Co-authored-by: Branislav Kontur <[email protected]> * Update polkadot/xcm/pallet-xcm/src/lib.rs Co-authored-by: Branislav Kontur <[email protected]> * Update polkadot/xcm/pallet-xcm/src/lib.rs Co-authored-by: Branislav Kontur <[email protected]> * Update polkadot/xcm/pallet-xcm/src/lib.rs Co-authored-by: Branislav Kontur <[email protected]> * fmt * Update bridge * Add WeightToFee to inbound queue config * remove comments * Add back treasury account * fix warnings * Update for benchmark * Update for benchmark * xcm-emulator: configure penpal for asset transfers and enhance existing tests * xcm-emulator: add relay to penpal native transfer test * xcm-emulator: add ah to penpal native asset transfer test * xcm: MultiLocation::chain_location() takes nonmut reference * pallet-xcm: benchmarks: enforce single asset transfer at the api level * xcm-executor: rename AssetTransferSupport to XcmAssetTransfer * clippy * fixes * remove duplicated trait * AssetHub tests: account for Westend higher delivery fees * fix merge damage * Added withdraw reserve to scripts * bridge-hub-westend-runtime: fix benchmarks * unknown import * move AllowSiblingsOnly to core * Adds transfer token user fee (#7) * adds transfer token user fee * updates fee calc * starts with adding snowbridge conf * updates fee * correct ROC amount * correct ROC amount --------- Co-authored-by: claravanstaden <Cats 4 life!> * Improve xcm integration test (#13) * Fix tests * Improve integration tests * Add weight for update_fees * remove gateway contract location * fmt * remove todos * use the correct bridge config * Enable utility calls * xcm-emulator: add ah to penpal multiple mixed assets transfer test * fix merge damage * remove limited from test names, all transfers use limited method now * barriers: allow withdrawing multiple assets in AllowTopLevelPaidExecutionFrom * Rename to set_token_transfer_fees * Adds Ethereum router (#16) * configures ethereum router * adds benchmarks and fmt --------- Co-authored-by: claravanstaden <Cats 4 life!> * fixed warning * pallet-xcm: add trace logs * fixes after merge * fix runtime-benchmarks * updated exporter * pallet-xcm: fix tests * increase fee * tweaked fee * xcm-emulator: remove unused pallet import * xcm-barrier: enforce MAX_ASSETS_FOR_BUY_EXECUTION * Change to Ethereum sovereign * chore * Tests for token transfer * fix tests * Support for message ids * rename some variables and fns * add explicit incomplete local execution error * rename fns * pallet-xcm: fix pallet extrinsic default weights * fix some errors * Update Cargo.lock * working on new bridge config * Fix assethub fee (#21) * updates fee after fixing decimals * adds fee in dot * adds fee in ksm * updates comment --------- Co-authored-by: claravanstaden <Cats 4 life!> * working on new bridge config * progress on config * pallet-xcm: also buy execution as part of custom fee handling * pallet-xcm: fix lossy fees math * fix merge damage * Add commons crate for BridgeHub runtime * fix merge damage * Fix emulated tests * fixes * attempting to fix message queue things * fix message queue things * fixes * Update Cargo.lock * re-use pallet * added fee handler trait * whitespace * move to treasury to snowbridge sovereign * remove treasury * remove TREASURY_PALLET_ID * remove unused imports * add fee trait * fix error * added full logic * adds tests * renamed trait * use siblings * address feedback * add snowbridge-runtime-common * fix build * cleanup * cleaner implementation * cargo lock * update cargo lock * latest updates and fmt * increases message queue sizes for beacon checkpoint * updates pallet indices * fixes * move to runtime folder * remove uneeded code * remove snowbridge sovereign from tests * use Balance * added refund * increased fee * Not going to refund treasuries * revert MessageQueueServiceWeight * update from polkadot upstream * update from polkadot and snowbridge upstream * fix merge damage * adds test back and fmt * add channel-id support * Fix breaking tests --------- Co-authored-by: Adrian Catangiu <[email protected]> Co-authored-by: claravanstaden <Cats 4 life!> Co-authored-by: Francisco Aguirre <[email protected]> Co-authored-by: Clara van Staden <[email protected]> Co-authored-by: Branislav Kontur <[email protected]> Co-authored-by: ron <[email protected]> Co-authored-by: Alistair Singh <[email protected]>

claravanstaden added 2 commits November 1, 2023 11:53

adds transfer token user fee

62b01ac

updates fee calc

8df73df

claravanstaden changed the title ~~adds transfer token user fee~~ Adds transfer token user fee Nov 1, 2023

claravanstaden commented Nov 1, 2023

View reviewed changes

claravanstaden requested review from vgeddes, alistair-singh and yrong November 1, 2023 11:38

claravanstaden marked this pull request as ready for review November 1, 2023 11:38

vgeddes reviewed Nov 1, 2023

View reviewed changes

starts with adding snowbridge conf

2d8a3e3

claravanstaden mentioned this pull request Nov 2, 2023

Adds transfer token user fee Snowfork/snowbridge#987

Merged

claravanstaden added 3 commits November 2, 2023 15:01

updates fee

b374803

correct ROC amount

3161ca5

correct ROC amount

dd43c0e

claravanstaden commented Nov 2, 2023

View reviewed changes

vgeddes approved these changes Nov 2, 2023

View reviewed changes

claravanstaden merged commit 55305d2 into reserve-asset-transfer Nov 2, 2023
5 of 11 checks passed

claravanstaden deleted the clara/sno-732-assethub-user-fee branch November 2, 2023 18:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds transfer token user fee #7

Adds transfer token user fee #7

claravanstaden commented Nov 1, 2023 •

edited

Loading

claravanstaden Nov 1, 2023

vgeddes Nov 1, 2023

vgeddes Nov 1, 2023

vgeddes Nov 1, 2023

vgeddes Nov 1, 2023

vgeddes Nov 1, 2023 •

edited

Loading

claravanstaden Nov 2, 2023

vgeddes Nov 2, 2023

vgeddes Nov 2, 2023

claravanstaden Nov 2, 2023

claravanstaden Nov 2, 2023

Adds transfer token user fee #7

Adds transfer token user fee #7

Conversation

claravanstaden commented Nov 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vgeddes Nov 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

claravanstaden commented Nov 1, 2023 •

edited

Loading

vgeddes Nov 1, 2023 •

edited

Loading