From 90837d95a83a5feea7781da1cf7b7b29b872d314 Mon Sep 17 00:00:00 2001 From: Nuno Alexandre Date: Mon, 11 Sep 2023 16:59:55 +0200 Subject: [PATCH] Fix lp-gateway routers' weight and fee calculations (#1537) * Fix lp-gateway routers' weight and fee calculations And adjust the integration tests to use reasonable amounts and verify the fees being charged. * lp-gateway-routers: Refactor XcmDomain * Fix tests * dev: Bump spec_version to 1022 * Drop commented old code --- .../routers/src/lib.rs | 50 ++---- .../routers/src/tests.rs | 158 +++--------------- runtime/development/src/lib.rs | 2 +- .../src/liquidity_pools/gateway.rs | 4 +- .../development/tests/liquidity_pools.rs | 36 +++- .../development/tests/routers/ethereum_xcm.rs | 8 +- 6 files changed, 81 insertions(+), 177 deletions(-) diff --git a/pallets/liquidity-pools-gateway/routers/src/lib.rs b/pallets/liquidity-pools-gateway/routers/src/lib.rs index 481d2b016d..e8a5a20445 100644 --- a/pallets/liquidity-pools-gateway/routers/src/lib.rs +++ b/pallets/liquidity-pools-gateway/routers/src/lib.rs @@ -38,7 +38,7 @@ use frame_system::pallet_prelude::OriginFor; use pallet_xcm_transactor::{Currency, CurrencyPayment, TransactWeights}; use scale_info::TypeInfo; use sp_core::{bounded::BoundedVec, ConstU32, H160, H256, U256}; -use sp_runtime::traits::{BlakeTwo256, EnsureMul, Hash}; +use sp_runtime::traits::{BlakeTwo256, Hash}; use sp_std::{boxed::Box, marker::PhantomData, vec::Vec}; use xcm::{ latest::{MultiLocation, OriginKind}, @@ -237,30 +237,6 @@ where let ethereum_xcm_call = get_encoded_ethereum_xcm_call::(self.xcm_domain.clone(), msg) .map_err(|_| DispatchError::Other("encoded ethereum xcm call retrieval"))?; - // Note: We are using moonbeams calculation for the ref time here and their - // estimate for the PoV. - // - // - Transact weight: gasLimit * 25000 as moonbeam is doing (Proof size - // limited fixed) - let transact_required_weight_at_most = Weight::from_ref_time( - self.xcm_domain - .max_gas_limit - .ensure_mul(GAS_TO_WEIGHT_MULTIPLIER)?, - ) - .set_proof_size(DEFAULT_PROOF_SIZE.saturating_div(2)); - - // NOTE: We are choosing an overall weight here to have full control over - // the actual weight usage. - // - // - Transact weight: gasLimit * 25000 as moonbeam is doing (Proof size - // limited fixed) - // - Weight for XCM instructions: Fixed weight * 3 (as we have 3 - // instructions) - let overall_weight = Weight::from_ref_time( - transact_required_weight_at_most.ref_time() + XCM_INSTRUCTION_WEIGHT * 3, - ) - .set_proof_size(DEFAULT_PROOF_SIZE); - pallet_xcm_transactor::Pallet::::transact_through_sovereign( ::RuntimeOrigin::root(), // The destination to which the message should be sent. @@ -270,16 +246,14 @@ where // The currency in which we want to pay fees. CurrencyPayment { currency: Currency::AsCurrencyId(self.xcm_domain.fee_currency.clone()), - fee_amount: Some( - self.xcm_domain.fee_per_second * Into::::into(overall_weight.ref_time()), - ), + fee_amount: Some(self.xcm_domain.fee_amount), }, // The call to be executed in the destination chain. ethereum_xcm_call, OriginKind::SovereignAccount, TransactWeights { - transact_required_weight_at_most, - overall_weight: Some(overall_weight), + transact_required_weight_at_most: self.xcm_domain.transact_required_weight_at_most, + overall_weight: Some(self.xcm_domain.overall_weight), }, )?; @@ -336,16 +310,22 @@ pub struct XcmDomain { /// The target contract address on a given domain. pub contract_address: H160, - /// The max gas_limit we want to propose for a remote evm execution + /// The max gas limit for the execution of the EVM call pub max_gas_limit: u64, + /// The max weight we want to pay for the execution of the Transact call in + /// the destination chain + pub transact_required_weight_at_most: Weight, + + /// The overall max weight we want to pay for the whole XCM message (i.e, + /// all the instructions) + pub overall_weight: Weight, + /// The currency in which execution fees will be paid on pub fee_currency: CurrencyId, - /// The fee per second that will be multiplied with - /// the overall weight of the call to define the fees on the - /// chain that will execute the call. - pub fee_per_second: u128, + /// The fee we use to buy execution for the execution of the Transact call + pub fee_amount: u128, } #[derive(Debug, Encode, Decode, Clone, PartialEq, Eq, TypeInfo, MaxEncodedLen)] diff --git a/pallets/liquidity-pools-gateway/routers/src/tests.rs b/pallets/liquidity-pools-gateway/routers/src/tests.rs index c2e0468ffa..53fe01e991 100644 --- a/pallets/liquidity-pools-gateway/routers/src/tests.rs +++ b/pallets/liquidity-pools-gateway/routers/src/tests.rs @@ -1,7 +1,7 @@ use ::xcm::{ lts::WeightLimit, v2::OriginKind, - v3::{Instruction::*, MultiAsset, Weight}, + v3::{Instruction::*, MultiAsset}, }; use cfg_mocks::MessageMock; use cfg_primitives::CFG; @@ -202,8 +202,10 @@ mod xcm_router { ethereum_xcm_transact_call_index: bounded_vec![0], contract_address: H160::from_slice([0; 20].as_slice()), max_gas_limit: 10, + transact_required_weight_at_most: Default::default(), + overall_weight: Default::default(), fee_currency: currency_id.clone(), - fee_per_second: 1u128, + fee_amount: 200000000000000000, }; let sender: AccountId32 = [0; 32].into(); @@ -256,24 +258,11 @@ mod xcm_router { let sent_messages = sent_xcm(); assert_eq!(sent_messages.len(), 1); - let transact_weight = Weight::from_parts( - test_data.xcm_domain.max_gas_limit * GAS_TO_WEIGHT_MULTIPLIER, - DEFAULT_PROOF_SIZE.saturating_div(2), - ); - - let overall_weight = Weight::from_parts( - transact_weight.ref_time() + XCM_INSTRUCTION_WEIGHT * 3, - DEFAULT_PROOF_SIZE, - ); - - let fees = Into::::into(overall_weight.ref_time()) - * test_data.xcm_domain.fee_per_second; - let (_, xcm) = sent_messages.first().unwrap(); assert!(xcm.0.contains(&WithdrawAsset( (MultiAsset { id: ::xcm::v3::AssetId::Concrete(MultiLocation::here()), - fun: ::xcm::v3::Fungibility::Fungible(fees), + fun: ::xcm::v3::Fungibility::Fungible(test_data.xcm_domain.fee_amount), }) .into() ))); @@ -281,9 +270,9 @@ mod xcm_router { assert!(xcm.0.contains(&BuyExecution { fees: MultiAsset { id: ::xcm::v3::AssetId::Concrete(MultiLocation::here()), - fun: ::xcm::v3::Fungibility::Fungible(fees), + fun: ::xcm::v3::Fungibility::Fungible(test_data.xcm_domain.fee_amount), }, - weight_limit: WeightLimit::Limited(overall_weight), + weight_limit: WeightLimit::Limited(test_data.xcm_domain.overall_weight), })); let expected_call = get_encoded_ethereum_xcm_call::( @@ -294,7 +283,7 @@ mod xcm_router { assert!(xcm.0.contains(&Transact { origin_kind: OriginKind::SovereignAccount, - require_weight_at_most: transact_weight, + require_weight_at_most: test_data.xcm_domain.transact_required_weight_at_most, call: expected_call.into(), })); }); @@ -315,29 +304,6 @@ mod xcm_router { assert_ok!(router.do_send(test_data.sender, test_data.msg)); }); } - - #[test] - fn transactor_info_not_set() { - new_test_ext().execute_with(|| { - let test_data = get_test_data(); - - let router = XCMRouter:: { - xcm_domain: test_data.xcm_domain.clone(), - _marker: Default::default(), - }; - - // Manually insert the fee per second in the `DestinationAssetFeePerSecond` - // storage. - - pallet_xcm_transactor::DestinationAssetFeePerSecond::::insert( - test_data.dest, - test_data.xcm_domain.fee_per_second.clone(), - ); - - // We ensure we can send although no `TransactInfo is set` - assert_ok!(router.do_send(test_data.sender, test_data.msg),); - }); - } } } @@ -549,8 +515,10 @@ mod axelar_xcm { ethereum_xcm_transact_call_index: bounded_vec![0], contract_address: H160::from_slice([0; 20].as_slice()), max_gas_limit: 10, + transact_required_weight_at_most: Default::default(), + overall_weight: Default::default(), fee_currency: currency_id.clone(), - fee_per_second: 1u128, + fee_amount: 200000000000000000, }; let axelar_target_chain = TEST_EVM_CHAIN.clone(); let axelar_target_contract = H160::from_low_u64_be(1); @@ -621,24 +589,14 @@ mod axelar_xcm { let sent_messages = sent_xcm(); assert_eq!(sent_messages.len(), 1); - let transact_weight = Weight::from_parts( - test_data.xcm_domain.max_gas_limit * GAS_TO_WEIGHT_MULTIPLIER, - DEFAULT_PROOF_SIZE.saturating_div(2), - ); - - let overall_weight = Weight::from_parts( - transact_weight.ref_time() + XCM_INSTRUCTION_WEIGHT * 3, - DEFAULT_PROOF_SIZE, - ); - - let fees = Into::::into(overall_weight.ref_time()) - * test_data.xcm_domain.fee_per_second; + let sent_messages = sent_xcm(); + assert_eq!(sent_messages.len(), 1); let (_, xcm) = sent_messages.first().unwrap(); assert!(xcm.0.contains(&WithdrawAsset( (MultiAsset { id: ::xcm::v3::AssetId::Concrete(MultiLocation::here()), - fun: ::xcm::v3::Fungibility::Fungible(fees), + fun: ::xcm::v3::Fungibility::Fungible(test_data.xcm_domain.fee_amount), }) .into() ))); @@ -646,9 +604,9 @@ mod axelar_xcm { assert!(xcm.0.contains(&BuyExecution { fees: MultiAsset { id: ::xcm::v3::AssetId::Concrete(MultiLocation::here()), - fun: ::xcm::v3::Fungibility::Fungible(fees), + fun: ::xcm::v3::Fungibility::Fungible(test_data.xcm_domain.fee_amount), }, - weight_limit: WeightLimit::Limited(overall_weight), + weight_limit: WeightLimit::Limited(test_data.xcm_domain.overall_weight), })); let contract_call = get_axelar_encoded_msg( @@ -657,7 +615,6 @@ mod axelar_xcm { test_data.axelar_target_contract, ) .unwrap(); - let expected_call = get_encoded_ethereum_xcm_call::( test_data.xcm_domain.clone(), contract_call, @@ -666,40 +623,11 @@ mod axelar_xcm { assert!(xcm.0.contains(&Transact { origin_kind: OriginKind::SovereignAccount, - require_weight_at_most: transact_weight, + require_weight_at_most: test_data.xcm_domain.transact_required_weight_at_most, call: expected_call.into(), })); }); } - - #[test] - fn transactor_info_not_set() { - new_test_ext().execute_with(|| { - let test_data = get_test_data(); - - let domain_router = - DomainRouter::::AxelarXCM(AxelarXCMRouter:: { - router: XCMRouter { - xcm_domain: test_data.xcm_domain.clone(), - _marker: Default::default(), - }, - axelar_target_chain: test_data.axelar_target_chain, - axelar_target_contract: test_data.axelar_target_contract, - _marker: Default::default(), - }); - - // Manually insert the fee per second in the `DestinationAssetFeePerSecond` - // storage. - - pallet_xcm_transactor::DestinationAssetFeePerSecond::::insert( - test_data.dest, - test_data.xcm_domain.fee_per_second.clone(), - ); - - // We ensure we can send although no `TransactInfo is set` - assert_ok!(domain_router.send(test_data.sender, test_data.msg),); - }); - } } } @@ -730,8 +658,10 @@ mod ethereum_xcm { ethereum_xcm_transact_call_index: bounded_vec![0], contract_address: H160::from_slice([0; 20].as_slice()), max_gas_limit: 10, + transact_required_weight_at_most: Default::default(), + overall_weight: Default::default(), fee_currency: currency_id.clone(), - fee_per_second: 1u128, + fee_amount: 200000000000000000, }; let axelar_target_chain = TEST_EVM_CHAIN.clone(); let axelar_target_contract = H160::from_low_u64_be(1); @@ -798,24 +728,11 @@ mod ethereum_xcm { let sent_messages = sent_xcm(); assert_eq!(sent_messages.len(), 1); - let transact_weight = Weight::from_parts( - test_data.xcm_domain.max_gas_limit * GAS_TO_WEIGHT_MULTIPLIER, - DEFAULT_PROOF_SIZE.saturating_div(2), - ); - - let overall_weight = Weight::from_parts( - transact_weight.ref_time() + XCM_INSTRUCTION_WEIGHT * 3, - DEFAULT_PROOF_SIZE, - ); - - let fees = Into::::into(overall_weight.ref_time()) - * test_data.xcm_domain.fee_per_second; - let (_, xcm) = sent_messages.first().unwrap(); assert!(xcm.0.contains(&WithdrawAsset( (MultiAsset { id: ::xcm::v3::AssetId::Concrete(MultiLocation::here()), - fun: ::xcm::v3::Fungibility::Fungible(fees), + fun: ::xcm::v3::Fungibility::Fungible(test_data.xcm_domain.fee_amount), }) .into() ))); @@ -823,9 +740,9 @@ mod ethereum_xcm { assert!(xcm.0.contains(&BuyExecution { fees: MultiAsset { id: ::xcm::v3::AssetId::Concrete(MultiLocation::here()), - fun: ::xcm::v3::Fungibility::Fungible(fees), + fun: ::xcm::v3::Fungibility::Fungible(test_data.xcm_domain.fee_amount), }, - weight_limit: WeightLimit::Limited(overall_weight), + weight_limit: WeightLimit::Limited(test_data.xcm_domain.overall_weight), })); let contract_call = get_encoded_contract_call(test_data.msg.serialize()).unwrap(); @@ -837,37 +754,10 @@ mod ethereum_xcm { assert!(xcm.0.contains(&Transact { origin_kind: OriginKind::SovereignAccount, - require_weight_at_most: transact_weight, + require_weight_at_most: test_data.xcm_domain.transact_required_weight_at_most, call: expected_call.into(), })); }); } - - #[test] - fn transactor_info_not_set() { - new_test_ext().execute_with(|| { - let test_data = get_test_data(); - - let domain_router = - DomainRouter::::EthereumXCM(EthereumXCMRouter:: { - router: XCMRouter { - xcm_domain: test_data.xcm_domain.clone(), - _marker: Default::default(), - }, - _marker: Default::default(), - }); - - // Manually insert the fee per second in the `DestinationAssetFeePerSecond` - // storage. - - pallet_xcm_transactor::DestinationAssetFeePerSecond::::insert( - test_data.dest, - test_data.xcm_domain.fee_per_second.clone(), - ); - - // We ensure we can send although no `TransactInfo is set` - assert_ok!(domain_router.send(test_data.sender, test_data.msg),); - }); - } } } diff --git a/runtime/development/src/lib.rs b/runtime/development/src/lib.rs index e02df6c836..082fb149a9 100644 --- a/runtime/development/src/lib.rs +++ b/runtime/development/src/lib.rs @@ -138,7 +138,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion { spec_name: create_runtime_str!("centrifuge-devel"), impl_name: create_runtime_str!("centrifuge-devel"), authoring_version: 1, - spec_version: 1021, + spec_version: 1022, impl_version: 1, #[cfg(not(feature = "disable-runtime-api"))] apis: RUNTIME_API_VERSIONS, diff --git a/runtime/integration-tests/src/liquidity_pools/gateway.rs b/runtime/integration-tests/src/liquidity_pools/gateway.rs index b85a576c4b..3b61179791 100644 --- a/runtime/integration-tests/src/liquidity_pools/gateway.rs +++ b/runtime/integration-tests/src/liquidity_pools/gateway.rs @@ -100,8 +100,10 @@ async fn set_router() { ethereum_xcm_transact_call_index: bounded_vec![0], contract_address: H160::from_low_u64_be(3), max_gas_limit: 10, + transact_required_weight_at_most: Default::default(), + overall_weight: Default::default(), fee_currency: currency_id, - fee_per_second: 1u128, + fee_amount: 0, }; let ethereum_xcm_router = EthereumXCMRouter:: { diff --git a/runtime/integration-tests/src/liquidity_pools/pallet/development/tests/liquidity_pools.rs b/runtime/integration-tests/src/liquidity_pools/pallet/development/tests/liquidity_pools.rs index 922573ec86..7b94a383a3 100644 --- a/runtime/integration-tests/src/liquidity_pools/pallet/development/tests/liquidity_pools.rs +++ b/runtime/integration-tests/src/liquidity_pools/pallet/development/tests/liquidity_pools.rs @@ -814,10 +814,28 @@ fn add_currency() { }) )); + assert_eq!( + OrmlTokens::free_balance( + GLIMMER_CURRENCY_ID, + &::Sender::get() + ), + DEFAULT_BALANCE_GLMR + ); + assert_ok!(LiquidityPools::add_currency( RuntimeOrigin::signed(BOB.into()), currency_id )); + + assert_eq!( + OrmlTokens::free_balance( + GLIMMER_CURRENCY_ID, + &::Sender::get() + ), + /// Ensure it only charged the 0.2 GLMR of fee + DEFAULT_BALANCE_GLMR + - dollar(18).saturating_div(5) + ); }); } @@ -1680,7 +1698,10 @@ mod utils { use cfg_types::tokens::CrossChainTransferability; use liquidity_pools_gateway_routers::{ ethereum_xcm::EthereumXCMRouter, DomainRouter, XCMRouter, XcmTransactInfo, + DEFAULT_PROOF_SIZE, }; + use runtime_common::xcm_fees::native_per_second; + use sp_runtime::traits::{EnsureDiv, EnsureMul}; use super::*; use crate::{ @@ -1688,7 +1709,8 @@ mod utils { utils::{AUSD_CURRENCY_ID, GLIMMER_CURRENCY_ID, MOONBEAM_EVM_CHAIN_ID}, }; - pub const DEFAULT_BALANCE_GLMR: Balance = 1_000_000_000_000; + // 10 GLMR (18 decimals) + pub const DEFAULT_BALANCE_GLMR: Balance = 10000000000000000000; pub const DOMAIN_MOONBEAM: Domain = Domain::EVM(MOONBEAM_EVM_CHAIN_ID); pub const DEFAULT_EVM_ADDRESS_MOONBEAM: [u8; 20] = [99; 20]; pub const DEFAULT_DOMAIN_ADDRESS_MOONBEAM: DomainAddress = @@ -1748,9 +1770,15 @@ mod utils { location: Box::new(xcm_domain_location), ethereum_xcm_transact_call_index: BoundedVec::truncate_from(vec![38, 0]), contract_address: H160::from(utils::DEFAULT_EVM_ADDRESS_MOONBEAM), - max_gas_limit: 700_000, + max_gas_limit: 500_000, + transact_required_weight_at_most: Weight::from_parts( + 12530000000, + DEFAULT_PROOF_SIZE.saturating_div(2), + ), + overall_weight: Weight::from_parts(15530000000, DEFAULT_PROOF_SIZE), fee_currency: currency_id, - fee_per_second: default_per_second(18), + // 0.2 token + fee_amount: 200000000000000000, }, _marker: Default::default(), }, @@ -1805,7 +1833,7 @@ mod utils { OrmlTokens::deposit( GLIMMER_CURRENCY_ID, &::Sender::get(), - DEFAULT_BALANCE_GLMR * dollar(18), + DEFAULT_BALANCE_GLMR, ); // Register AUSD in the asset registry which is the default pool currency in diff --git a/runtime/integration-tests/src/liquidity_pools/pallet/development/tests/routers/ethereum_xcm.rs b/runtime/integration-tests/src/liquidity_pools/pallet/development/tests/routers/ethereum_xcm.rs index c14debc5ba..fd9dd00c2a 100644 --- a/runtime/integration-tests/src/liquidity_pools/pallet/development/tests/routers/ethereum_xcm.rs +++ b/runtime/integration-tests/src/liquidity_pools/pallet/development/tests/routers/ethereum_xcm.rs @@ -98,8 +98,10 @@ fn get_axelar_xcm_router_fn() -> RouterCreationFn { ethereum_xcm_transact_call_index: BoundedVec::truncate_from(vec![38, 0]), contract_address: H160::from_low_u64_be(11), max_gas_limit: 700_000, + transact_required_weight_at_most: Default::default(), + overall_weight: Default::default(), fee_currency: currency_id, - fee_per_second: default_per_second(18), + fee_amount: dollar(18).saturating_div(5), }, _marker: Default::default(), }, @@ -123,8 +125,10 @@ fn get_ethereum_xcm_router_fn() -> RouterCreationFn { ethereum_xcm_transact_call_index: BoundedVec::truncate_from(vec![38, 0]), contract_address: H160::from_low_u64_be(11), max_gas_limit: 700_000, + transact_required_weight_at_most: Default::default(), + overall_weight: Default::default(), fee_currency: currency_id, - fee_per_second: default_per_second(18), + fee_amount: dollar(18).saturating_div(5), }, _marker: Default::default(), },