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.

Sign up for GitHub

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

add FeeManager to pallet xcm #5363

Merged
merged 16 commits into from
Nov 11, 2024
Merged

add FeeManager to pallet xcm #5363

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
a5fe494
add FeeManager to pallet xcm
jpserrat Aug 14, 2024
03f521e
remove unused import
jpserrat Aug 14, 2024
e8cba6e
fix fmt
jpserrat Aug 14, 2024
be37848
remove unused import
jpserrat Aug 14, 2024
c7b575a
refactor: impl FeeManager trait to XcmExecutor
jpserrat Aug 30, 2024
28ffa84
Merge branch 'master' of github.com:paritytech/polkadot-sdk into jpse…
jpserrat Aug 30, 2024
1cddea5
Merge branch 'master' of github.com:paritytech/polkadot-sdk into jpse…
jpserrat Aug 30, 2024
43da487
add root location to waived locations
jpserrat Nov 10, 2024
a08783e
return send_xcm to insert DecendOrigin to message when interior != Ju…
jpserrat Nov 10, 2024
74506e5
add prdoc for waive transport fees based on XcmConfig
jpserrat Nov 10, 2024
f5b6334
Merge branch 'master' of github.com:paritytech/polkadot-sdk into jpse…
jpserrat Nov 10, 2024
79ad2c6
fix send_xcm charge fees condition
jpserrat Nov 10, 2024
6762b4e
Update polkadot/xcm/pallet-xcm/src/lib.rs
acatangiu Nov 11, 2024
ddf5ac3
Merge branch 'master' into jpserrat/use-is-waived-on-send-xcm
acatangiu Nov 11, 2024
fbea24e
Update from acatangiu running command 'fmt'
actions-user Nov 11, 2024
94aa5c3
Merge branch 'master' into jpserrat/use-is-waived-on-send-xcm
acatangiu Nov 11, 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
8 changes: 7 additions & 1 deletion cumulus/parachains/runtimes/testing/penpal/src/xcm_config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,12 @@ pub type TrustedReserves = (
pub type TrustedTeleporters =
(AssetFromChain<LocalTeleportableToAssetHub, SystemAssetHubLocation>,);

pub struct WaivedLocations;
impl Contains<Location> for WaivedLocations {
fn contains(location: &Location) -> bool {
*location == Location::here()
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

There is already something you can use here:

Suggested change
pub struct WaivedLocations;
impl Contains<Location> for WaivedLocations {
fn contains(location: &Location) -> bool {
*location == Location::here()
}
}
pub type WaivedLocations = Equals<RootLocation>;

Also please make sure all the other system chain runtimes have Equals<RootLocation> part of their WaivedLocations so as not to break functionality.

Hint: they currently do not, for example collectives-westend has it, make sure the others do too.

pub struct XcmConfig;
impl xcm_executor::Config for XcmConfig {
type RuntimeCall = RuntimeCall;
Expand Down Expand Up @@ -354,7 +360,7 @@ impl xcm_executor::Config for XcmConfig {
type AssetLocker = ();
type AssetExchanger = ();
type FeeManager = XcmFeeManagerFromComponents<
(),
WaivedLocations,
SendXcmFeeToAccount<Self::AssetTransactor, TreasuryAccount>,
>;
type MessageExporter = ();
Expand Down
18 changes: 9 additions & 9 deletions polkadot/xcm/pallet-xcm/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ use xcm_runtime_apis::{

#[cfg(any(feature = "try-runtime", test))]
use sp_runtime::TryRuntimeError;
use xcm_executor::traits::{FeeManager, FeeReason};

pub trait WeightInfo {
fn send() -> Weight;
Expand Down Expand Up @@ -239,7 +240,7 @@ pub mod pallet {
type XcmExecuteFilter: Contains<(Location, Xcm<<Self as Config>::RuntimeCall>)>;

/// Something to execute an XCM message.
type XcmExecutor: ExecuteXcm<<Self as Config>::RuntimeCall> + XcmAssetTransfers;
type XcmExecutor: ExecuteXcm<<Self as Config>::RuntimeCall> + XcmAssetTransfers + FeeManager;

/// Our XCM filter which messages to be teleported using the dedicated extrinsic must pass.
type XcmTeleportFilter: Contains<(Location, Vec<Asset>)>;
Expand Down Expand Up @@ -2421,17 +2422,16 @@ impl<T: Config> Pallet<T> {
mut message: Xcm<()>,
) -> Result<XcmHash, SendError> {
let interior = interior.into();
let origin_dest = interior.clone().into();
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the origin as seen by the local chain, relative to Here.

Suggested change
let origin_dest = interior.clone().into();
let local_origin = interior.clone().into();

let dest = dest.into();
let maybe_fee_payer = if interior != Junctions::Here {
message.0.insert(0, DescendOrigin(interior.clone()));
Some(interior.into())
} else {
None
};
let is_waived = <T::XcmExecutor as FeeManager>::is_waived(Some(&origin_dest), FeeReason::ChargeFees);
if !is_waived {
message.0.insert(0, DescendOrigin(interior));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not really what we want.

Whether or not DescendOrigin(interior) is inserted is orthogonal to whether fees are paid; it should depend on the same condition as before:

Suggested change
if !is_waived {
message.0.insert(0, DescendOrigin(interior));
}
if interior != Junctions::Here {
message.0.insert(0, DescendOrigin(interior));
}

log::debug!(target: "xcm::send_xcm", "dest: {:?}, message: {:?}", &dest, &message);
let (ticket, price) = validate_send::<T::XcmRouter>(dest, message)?;
if let Some(fee_payer) = maybe_fee_payer {
Self::charge_fees(fee_payer, price).map_err(|_| SendError::Fees)?;
if !is_waived {
Self::charge_fees(origin_dest, price).map_err(|_| SendError::Fees)?;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This condition is good! 👍

Just rename the var

Suggested change
if !is_waived {
Self::charge_fees(origin_dest, price).map_err(|_| SendError::Fees)?;
}
if !is_waived {
Self::charge_fees(local_origin, price).map_err(|_| SendError::Fees)?;
}

T::XcmRouter::deliver(ticket)
}
Expand Down
10 changes: 10 additions & 0 deletions polkadot/xcm/xcm-executor/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,16 @@ impl<Config: config::Config> XcmAssetTransfers for XcmExecutor<Config> {
type AssetTransactor = Config::AssetTransactor;
}

impl <Config: config::Config> FeeManager for XcmExecutor<Config> {
fn is_waived(origin: Option<&Location>, r: FeeReason) -> bool {
Config::FeeManager::is_waived(origin, r)
}

fn handle_fee(fee: Assets, context: Option<&XcmContext>, r: FeeReason) {
Config::FeeManager::handle_fee(fee, context, r)
}
}

#[derive(Debug)]
pub struct ExecutorError {
pub index: u32,
Expand Down
Loading