add FeeManager to pallet xcm

[jpserrat]

Closes #2082

change send xcm to use xcm::executor::FeeManager to determine if the sender should be charged.

I had to change the FeeManager of the penpal config to ensure the same test behaviour as before. For the other tests, I'm using the FeeManager from the xcm::executor::FeeManager as this one is used to check if the fee can be waived on the charge fees method.

[acatangiu]

I am in favor of re-exposing FeeManager from XcmExecutor type instead of creating yet another configuration type here.

I don't see why pallet-xcm would be configured with fee manager different than the xcm-executor. The benefit outweighs the overhead cost of configuring it twice and thus also the risk of misconfiguring it.

type XcmExecutor here can take a trait FeeManager bound.

xcm-executor implementation can impl FeeManager where it would simply re-expose its inner configured type FeeManager.

This will help with ergonomics and allow most runtimes to ingest this change with no breakages and needs to adjust configs.

[jpserrat]

Done!

[paritytech-cicd-pr]

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable 1/3
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7201111

[acatangiu]

@jpserrat sorry this fell off my radar for a while.

Changes look mostly good, a couple of comments you need to address and rebase this against latest master.

[acatangiu]

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));
	}

[acatangiu]

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();

[acatangiu]

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)?;
	}

[acatangiu]

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.

[acatangiu]

You also need to add a prdoc/pr_5363.prdoc new file and describe the change.

Here is an example prdoc file

In this file you should add a clear:

🚨 Warning: 🚨 pallet-xcm::send() no longer implicitly waives transport fees for the local root location, but instead relies on xcm_executor::Config::FeeManager to determine whether certain locations have free transport.
If your chain relies on free transport for local root, please make sure to add Location::here() to the waived-fee locations in your configured xcm_executor::Config::FeeManager.

it should also do major bump to pallet-xcm because of the change in behavior:

crates:
  - name: pallet-xcm
    bump: major

[acatangiu]

/cmd fmt

[github-actions]

Command "fmt" has started 🚀 See logs here

[github-actions]

Command "fmt" has finished ✅ See logs here