`[pallet_xcm::reserve_transfer_assets]` `NotHoldingFees` issue in case of paid XcmRouter is used

[bkontur]

Based on PR: https://github.com/paritytech/polkadot/pull/7005

TL;DR: this PR:

• fixes respecting fees_mode from SetFeesMode correctly in all cases in XcmExecutor
• adds possibility to set up "worst case" scenario for FeeManager with withdrawing and handling fees for transfer_reserve_asset

---

---

---

Long story:

on AssetHubs we plan to set FeeManager and waive unpaid only for system parachains or relay chain https://github.com/paritytech/cumulus/pull/2433/files#diff-9a51956f7781a349d207a9d5f54ea0061666a856fb108d1a1190bd5178eaac50R432-R437 so it means, that any user/origin will be charged for delivery, from holding register:

                let (ticket, fee) = validate_send::<Config::XcmSender>(dest, msg)?;
		if !Config::FeeManager::is_waived(self.origin_ref(), reason) {
			let paid = self.holding.try_take(fee.into()).map_err(|_| XcmError::NotHoldingFees)?;
			Config::FeeManager::handle_fee(paid.into(), Some(&self.context));
		}
		Config::XcmSender::deliver(ticket).map_err(Into::into)

which seems ok, but there are several cases, when this holding register is not filled and is empty, for example pallet_xcm::reserve_transfer_assets generates instructions:

SetFeesMode { jit_withdraw: true },
TransferReserveAsset { assets, ...}

SetFeesMode -> does nothing, because it affects only sef.take_fee( which is used only for ExportMessage / LockAsset / RequestUnlock TransferReserveAsset - does reserve transfer of assets and then self.send(ReserveAssetDeposited(assets)) to destination

and here are problems with this self.send:

---

1. there is nothing in holding so this self.send for regular user fails on NotHoldingFees - see test bellow without fix

---

2. Should we withdraw fee from validate_send from user account directly or from holding register? maybe here in fn send is missing IF based on jit_withdraw: true, which is the same code as fn take_fee does?

so maybe solution is to use here self.take_fee( - which is working for test :)

/// Send an XCM, charging fees from Holding as needed.
	fn send(
		&mut self,
		dest: MultiLocation,
		msg: Xcm<()>,
		reason: FeeReason,
	) -> Result<XcmHash, XcmError> {
		let (ticket, fee) = validate_send::<Config::XcmSender>(dest, msg)?;
		if !Config::FeeManager::is_waived(self.origin_ref(), reason) {
			self.take_fee(fee, reason)?;
		}
		Config::XcmSender::deliver(ticket).map_err(Into::into)
	}

---

Test fails withtout changing:

orginal:

		if !Config::FeeManager::is_waived(self.origin_ref(), reason) {
			let paid = self.holding.try_take(fee.into()).map_err(|_| XcmError::NotHoldingFees)?;
			Config::FeeManager::handle_fee(paid.into(), Some(&self.context));
		}

fixed to:

		if !Config::FeeManager::is_waived(self.origin_ref(), reason) {
			self.take_fee(fee, reason)?;
		}

cargo test --package pallet-xcm --lib tests::reserve_transfer_assets_with_paid_router_works -- --exact

Left:  RuntimeEvent::XcmPallet(Event::Attempted { outcome: Incomplete(Weight { ref_time: 2000, proof_size: 2000 }, NotHoldingFees) })
Right: RuntimeEvent::XcmPallet(Event::Attempted { outcome: Complete(Weight { ref_time: 2000, proof_size: 2000 }) })
<Click to see difference>

thread 'tests::reserve_transfer_assets_with_paid_router_works' panicked at 'assertion failed: `(left == right)`
  left: `RuntimeEvent::XcmPallet(Event::Attempted { outcome: Incomplete(Weight { ref_time: 2000, proof_size: 2000 }, NotHoldingFees) })`,
 right: `RuntimeEvent::XcmPallet(Event::Attempted { outcome: Complete(Weight { ref_time: 2000, proof_size: 2000 }) })`', xcm/pallet-xcm/src/tests.rs:563:9

---

TODOs:

• [x] fix CI test mistery, because locally works (maybe some sync/thread_local issue)
• [x] check/fix benchmarks which depends on is_waived

[bkontur]

@joepetrowski @KiChjang additional question is, how can UI/UX display this additinal fees from XcmRouter generated here let (ticket, fee) = validate_send::< ?

maybe some additional runtime api which will be able to trigger just validate_send::< or any other solution?

[paritytech-cicd-pr]

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

[bkontur]

bot rebase

[paritytech-processbot[bot]]

Rebased

[bkontur]

bot rebase

[paritytech-processbot[bot]]

Rebased

[bkontur]

bot rebase

[paritytech-processbot[bot]]

Rebased

[bkontur]

bot fmt

[command-bot[bot]]

@bkontur https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/3404171 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 40-7e22532a-d9d9-456e-b45d-26ee707b510a to cancel this command or bot cancel to cancel all commands in this pull request.

[command-bot[bot]]

@bkontur Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/3404171 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/3404171/artifacts/download.

[bkontur]

Overall solution seems sound, however too much code duplication and bare logic is introduced into high-level runtime definition code which should be 99% declarative.

Something for all Frame coders to note:

The central lib.rs files of runtimes should not contain bare logic. The point of the extensive use of traits and types in how I designed Frame is to avoid exactly this. Runtimes should ONLY CONTAIN BASIC TYPE DEFINITIONS. Any logic should be parcelled up into one or more reusable types and stashed somewhere common for everyone else to make use of too.

@gavofyork I fixed and refactored those duplication like you suggested, could you please, check? I hesitate to merge without your explicit approve here (because you requested changes), thank you :)