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

Fix DepositReserveAsset fees payment #3340

Merged
merged 13 commits into from
Feb 23, 2024
2 changes: 2 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions polkadot/node/test/service/src/chain_spec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,7 @@ fn polkadot_testnet_genesis(
paras_availability_period: 4,
no_show_slots: 10,
minimum_validation_upgrade_delay: 5,
max_downward_message_size: 1024,
..Default::default()
},
}
Expand Down
54 changes: 42 additions & 12 deletions polkadot/runtime/test-runtime/src/xcm_config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,16 @@

use frame_support::{
parameter_types,
traits::{Everything, Nothing},
traits::{Everything, Get, Nothing},
weights::Weight,
};
use frame_system::EnsureRoot;
use polkadot_runtime_parachains::FeeTracker;
use runtime_common::xcm_sender::{ChildParachainRouter, PriceForMessageDelivery};
use xcm::latest::prelude::*;
use xcm_builder::{
AllowUnpaidExecutionFrom, EnsureXcmOrigin, FixedWeightBounds, FrameTransactionalProcessor,
SignedAccountId32AsNative, SignedToAccountId32,
SignedAccountId32AsNative, SignedToAccountId32, WithUniqueTopic,
};
use xcm_executor::{
traits::{TransactAsset, WeightTrader},
Expand All @@ -36,6 +38,8 @@ parameter_types! {
pub const MaxInstructions: u32 = 100;
pub const MaxAssetsIntoHolding: u32 = 16;
pub const UniversalLocation: xcm::latest::InteriorLocation = xcm::latest::Junctions::Here;
pub TokenLocation: Location = Here.into_location();
pub FeeAssetId: AssetId = AssetId(TokenLocation::get());
}

/// Type to convert an `Origin` type value into a `Location` value which represents an interior
Expand All @@ -45,17 +49,43 @@ pub type LocalOriginToLocation = (
SignedToAccountId32<crate::RuntimeOrigin, crate::AccountId, AnyNetwork>,
);

pub struct DoNothingRouter;
impl SendXcm for DoNothingRouter {
type Ticket = ();
fn validate(_dest: &mut Option<Location>, _msg: &mut Option<Xcm<()>>) -> SendResult<()> {
Ok(((), Assets::new()))
}
fn deliver(_: ()) -> Result<XcmHash, SendError> {
Ok([0; 32])
/// Implementation of [`PriceForMessageDelivery`], returning a different price
/// based on whether a message contains a reanchored asset or not.
/// This implementation ensures that messages with non-reanchored assets return higher
/// prices than messages with reanchored assets.
/// Useful for `deposit_reserve_asset_works_for_any_xcm_sender` integration test.
pub struct TestDeliveryPrice<A, F>(sp_std::marker::PhantomData<(A, F)>);
impl<A: Get<AssetId>, F: FeeTracker> PriceForMessageDelivery for TestDeliveryPrice<A, F> {
type Id = F::Id;

fn price_for_delivery(_: Self::Id, msg: &Xcm<()>) -> Assets {
let base_fee: super::Balance = 1_000_000;

let parents = msg.iter().find_map(|xcm| match xcm {
ReserveAssetDeposited(assets) => {
let AssetId(location) = &assets.inner().first().unwrap().id;
Some(location.parents)
},
_ => None,
});

// If no asset is found, price defaults to `base_fee`.
let amount = base_fee
.saturating_add(base_fee.saturating_mul(parents.unwrap_or(0) as super::Balance));

(A::get(), amount).into()
}
}

pub type PriceForChildParachainDelivery = TestDeliveryPrice<FeeAssetId, super::Dmp>;

/// The XCM router. When we want to send an XCM message, we use this type. It amalgamates all of our
/// individual routers.
pub type XcmRouter = WithUniqueTopic<
// Only one router so far - use DMP to communicate with child parachains.
ChildParachainRouter<super::Runtime, super::Xcm, PriceForChildParachainDelivery>,
>;

pub type Barrier = AllowUnpaidExecutionFrom<Everything>;

pub struct DummyAssetTransactor;
Expand Down Expand Up @@ -99,7 +129,7 @@ type OriginConverter = (
pub struct XcmConfig;
impl xcm_executor::Config for XcmConfig {
type RuntimeCall = super::RuntimeCall;
type XcmSender = DoNothingRouter;
type XcmSender = XcmRouter;
type AssetTransactor = DummyAssetTransactor;
type OriginConverter = OriginConverter;
type IsReserve = ();
Expand Down Expand Up @@ -133,7 +163,7 @@ impl pallet_xcm::Config for crate::Runtime {
type UniversalLocation = UniversalLocation;
type SendXcmOrigin = EnsureXcmOrigin<crate::RuntimeOrigin, LocalOriginToLocation>;
type Weigher = FixedWeightBounds<BaseXcmWeight, crate::RuntimeCall, MaxInstructions>;
type XcmRouter = DoNothingRouter;
type XcmRouter = XcmRouter;
type XcmExecuteFilter = Everything;
type XcmExecutor = xcm_executor::XcmExecutor<XcmConfig>;
type XcmTeleportFilter = Everything;
Expand Down
2 changes: 2 additions & 0 deletions polkadot/xcm/xcm-executor/integration-tests/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,15 @@ pallet-xcm = { path = "../../pallet-xcm" }
polkadot-test-client = { path = "../../../node/test/client" }
polkadot-test-runtime = { path = "../../../runtime/test-runtime" }
polkadot-test-service = { path = "../../../node/test/service" }
polkadot-service = { path = "../../../node/service" }
sp-consensus = { path = "../../../../substrate/primitives/consensus/common" }
sp-keyring = { path = "../../../../substrate/primitives/keyring" }
sp-runtime = { path = "../../../../substrate/primitives/runtime", default-features = false }
sp-state-machine = { path = "../../../../substrate/primitives/state-machine" }
xcm = { package = "staging-xcm", path = "../..", default-features = false }
xcm-executor = { package = "staging-xcm-executor", path = ".." }
sp-tracing = { path = "../../../../substrate/primitives/tracing" }
sp-core = { path = "../../../../substrate/primitives/core" }

[features]
default = ["std"]
Expand Down
83 changes: 83 additions & 0 deletions polkadot/xcm/xcm-executor/integration-tests/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,14 @@

use codec::Encode;
use frame_support::{dispatch::GetDispatchInfo, weights::Weight};
use polkadot_service::chain_spec::get_account_id_from_seed;
use polkadot_test_client::{
BlockBuilderExt, ClientBlockImportExt, DefaultTestClientBuilderExt, InitPolkadotBlockBuilder,
TestClientBuilder, TestClientBuilderExt,
};
use polkadot_test_runtime::{pallet_test_notifier, xcm_config::XcmConfig};
use polkadot_test_service::construct_extrinsic;
use sp_core::sr25519;
use sp_runtime::traits::Block;
use sp_state_machine::InspectState;
use xcm::{latest::prelude::*, VersionedResponse, VersionedXcm};
Expand Down Expand Up @@ -323,3 +325,84 @@ fn query_response_elicits_handler() {
)));
});
}

/// Simulates a cross-chain message from Parachain to Parachain through Relay Chain
/// that deposits assets into the reserve of the destination.
/// Regression test for `DepostiReserveAsset` changes in
/// <https://github.com/paritytech/polkadot-sdk/pull/3340>
#[test]
fn deposit_reserve_asset_works_for_any_xcm_sender() {
Copy link
Contributor

Choose a reason for hiding this comment

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

this new test is failing in CI following merge from master

Copy link
Contributor Author

@NachoPal NachoPal Feb 22, 2024

Choose a reason for hiding this comment

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

The previous PriceForMessageDelivery was flaky returning different prices.

Primarily, it was designed to return the same price for identical messages. However, reanchored messages weren't actually the same.

  • First price calculation is done for a message with the full asset amount in ReserveAssetDeposited
  • Actual price calculation during send() is done for a message where transport_fee has been deducted from the initial asset amount.

It does not explain by itself why the test was flaky. The reason for this flakiness is that the appended SetTopic(id) is calculated using the previous block as entropy. This is why any change in the runtime led to changes in the final message which might or might not generate enough transport fees to be paid.

All of this made me realise that the current method of setting aside the transport_fee for DepositReserveAsset will never be infallible for any possible PriceForMessageDelivery implementation. There is a circular dependency where it is impossible to know the actual DepositReserveAsset amount that will finally be included in the message from which the transport_fee is calculated. @acatangiu @franciscoaguirre

I followed a different approach to fix the test.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you expand a bit on the transport cost variance?

The actual amount (scalar value) of the asset within the message changes/impacts the message weight? I.e. An instruction containing asset: 2 DOT is more expensive than same instruction with asset: 3 DOT? Did I understand that correctly?
Or is it an issue with SetTopic(id) whose transport cost is not correctly calculated during execution of ReserveAssetDeposited? Looking at the code I'm missing where this cost changes between when we calculate and when it is charged..

Copy link
Contributor Author

@NachoPal NachoPal Feb 23, 2024

Choose a reason for hiding this comment

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

An instruction containing asset: 2 DOT is more expensive than same instruction with asset: 3 DOT? Did I understand that correctly?

Not for our PriceForMessageDelivery implementation, because it only depends on the message size, that is why it was failing when the asset was not reanchored.

I am just saying that potentially, a third party could make use of the xcm-executor with a different PriceForMessageDelivery implementation, and we can not guarantee that the set aside transport_fee approach for DepositReserveAsset will be infalible. It is just something to be aware of in case the design can be improved, not a big deal, not even probable to ever happen, and something that shouldn't block this PR.

Copy link
Contributor Author

@NachoPal NachoPal Feb 23, 2024

Choose a reason for hiding this comment

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

I'll proceed to merge it if the new test looks ok to you.

sp_tracing::try_init_simple();
let mut client = TestClientBuilder::new().build();

// Init values for the simulated origin Parachain
let amount_to_send: u128 = 1_000_000_000_000;
let assets: Assets = (Parent, amount_to_send).into();
let fee_asset_item = 0;
let max_assets = assets.len() as u32;
let fees = assets.get(fee_asset_item as usize).unwrap().clone();
let weight_limit = Unlimited;
let reserve = Location::parent();
let dest = Location::new(1, [Parachain(2000)]);
let beneficiary_id = get_account_id_from_seed::<sr25519::Public>("Alice");
let beneficiary = Location::new(0, [AccountId32 { network: None, id: beneficiary_id.into() }]);

// spends up to half of fees for execution on reserve and other half for execution on
// destination
let fee1 = amount_to_send.saturating_div(2);
let fee2 = amount_to_send.saturating_sub(fee1);
let fees_half_1 = Asset::from((fees.id.clone(), Fungible(fee1)));
let fees_half_2 = Asset::from((fees.id.clone(), Fungible(fee2)));

let reserve_context = <XcmConfig as xcm_executor::Config>::UniversalLocation::get();
// identifies fee item as seen by `reserve` - to be used at reserve chain
let reserve_fees = fees_half_1.reanchored(&reserve, &reserve_context).unwrap();
// identifies fee item as seen by `dest` - to be used at destination chain
let dest_fees = fees_half_2.reanchored(&dest, &reserve_context).unwrap();
// identifies assets as seen by `reserve` - to be used at reserve chain
let assets_reanchored = assets.reanchored(&reserve, &reserve_context).unwrap();
// identifies `dest` as seen by `reserve`
let dest = dest.reanchored(&reserve, &reserve_context).unwrap();
// xcm to be executed at dest
let xcm_on_dest = Xcm(vec![
BuyExecution { fees: dest_fees, weight_limit: weight_limit.clone() },
DepositAsset { assets: Wild(AllCounted(max_assets)), beneficiary },
]);
// xcm to be executed at reserve
let msg = Xcm(vec![
WithdrawAsset(assets_reanchored),
ClearOrigin,
BuyExecution { fees: reserve_fees, weight_limit },
DepositReserveAsset { assets: Wild(AllCounted(max_assets)), dest, xcm: xcm_on_dest },
]);

let mut block_builder = client.init_polkadot_block_builder();

// Simulate execution of an incoming XCM message at the reserve chain
let execute = construct_extrinsic(
&client,
polkadot_test_runtime::RuntimeCall::Xcm(pallet_xcm::Call::execute {
message: Box::new(VersionedXcm::from(msg)),
max_weight: Weight::from_parts(1_000_000_000, 1024 * 1024),
}),
sp_keyring::Sr25519Keyring::Alice,
0,
);

block_builder.push_polkadot_extrinsic(execute).expect("pushes extrinsic");

let block = block_builder.build().expect("Finalizes the block").block;
let block_hash = block.hash();

futures::executor::block_on(client.import(sp_consensus::BlockOrigin::Own, block))
.expect("imports the block");

client.state_at(block_hash).expect("state should exist").inspect_state(|| {
assert!(polkadot_test_runtime::System::events().iter().any(|r| matches!(
r.event,
polkadot_test_runtime::RuntimeEvent::Xcm(pallet_xcm::Event::Attempted {
outcome: Outcome::Complete { .. }
}),
)));
});
}
4 changes: 2 additions & 2 deletions polkadot/xcm/xcm-executor/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -826,9 +826,9 @@ impl<Config: config::Config> XcmExecutor<Config> {
// be weighed
let to_weigh = self.holding.saturating_take(assets.clone());
self.holding.subsume_assets(to_weigh.clone());

let to_weigh_reanchored = Self::reanchored(to_weigh, &dest, None);
let mut message_to_weigh =
vec![ReserveAssetDeposited(to_weigh.into()), ClearOrigin];
vec![ReserveAssetDeposited(to_weigh_reanchored), ClearOrigin];
message_to_weigh.extend(xcm.0.clone().into_iter());
let (_, fee) =
validate_send::<Config::XcmSender>(dest.clone(), Xcm(message_to_weigh))?;
Expand Down
Loading