Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

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

Open
wants to merge 18 commits into
base: kckyeung/xcm-fee-manager
Choose a base branch
from

Conversation

bkontur
Copy link
Contributor

@bkontur bkontur commented Aug 7, 2023

Based on PR: #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

  1. 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:

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

@bkontur
Copy link
Contributor Author

bkontur commented Aug 7, 2023

@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
Copy link

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

Copy link
Contributor

@KiChjang KiChjang left a comment

Choose a reason for hiding this comment

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

I think the intention is that any fees payable in the XCM executor can be configured via SetFeesMode, but some of the existing code were not changed to respect the fees_mode register, so this change LGTM.

@paritytech-ci paritytech-ci requested review from a team August 9, 2023 15:17
@paritytech-ci paritytech-ci requested review from a team August 10, 2023 16:45
@paritytech-ci paritytech-ci requested review from a team August 10, 2023 16:45
Copy link
Member

@gavofyork gavofyork left a comment

Choose a reason for hiding this comment

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

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.

@paritytech-ci paritytech-ci requested a review from a team August 10, 2023 16:48
@bkontur bkontur requested review from a team and chevdor as code owners August 13, 2023 12:20
@bkontur bkontur requested a review from gavofyork August 13, 2023 12:25
@bkontur
Copy link
Contributor Author

bkontur commented Aug 13, 2023

bot rebase

@paritytech-processbot
Copy link

Rebased

@bkontur
Copy link
Contributor Author

bkontur commented Aug 15, 2023

bot rebase

@paritytech-processbot
Copy link

Rebased

@bkontur
Copy link
Contributor Author

bkontur commented Aug 16, 2023

bot rebase

@paritytech-processbot
Copy link

Rebased

@bkontur
Copy link
Contributor Author

bkontur commented Aug 16, 2023

bot fmt

@command-bot
Copy link

command-bot bot commented Aug 16, 2023

@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
Copy link

command-bot bot commented Aug 16, 2023

@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
Copy link
Contributor Author

bkontur commented Aug 18, 2023

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 :)

Copy link
Contributor

@franciscoaguirre franciscoaguirre left a comment

Choose a reason for hiding this comment

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

Just some nits

/// Deposits estimated fee to the origin account (if needed).
/// Allows to trigger additional logic for specific `ParaId` (e.g. open HRMP channel) (if neeeded).
#[cfg(feature = "runtime-benchmarks")]
pub struct ToParachainDeliveryHelper<
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be here or in xcm-builder?

Comment on lines +198 to +205
parameter_types! {
pub Para3000: u32 = 3000;
pub Para3000Location: MultiLocation = Parachain(Para3000::get()).into();
pub Para3000PaymentAmount: u128 = 1;
pub Para3000PaymentMultiAssets: MultiAssets = MultiAssets::from(MultiAsset::from((Here, Para3000PaymentAmount::get())));
}
/// Sender only sends to `Parachain(3000)` destination requiring payment.
pub struct TestPaidForPara3000SendXcm;
Copy link
Contributor

Choose a reason for hiding this comment

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

It feels like these items are specific to a particular test. Why not put them in tests rather than in mock? The test sender should be generic enough.

@@ -346,7 +348,8 @@ impl xcm_executor::Config for XcmConfig {
type SubscriptionService = XcmPallet;
type PalletInstancesInfo = AllPalletsWithSystem;
type MaxAssetsIntoHolding = MaxAssetsIntoHolding;
type FeeManager = XcmFeesToAccount<Self, SystemParachains, AccountId, TreasuryAccount>;
type FeeManager =
XcmFeesToAccount<Self, (SystemParachains, HereLocation), AccountId, TreasuryAccount>;
Copy link
Member

Choose a reason for hiding this comment

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

Please explain.

@paritytech-ci paritytech-ci requested review from a team August 25, 2023 04:33
Copy link
Member

@gavofyork gavofyork left a comment

Choose a reason for hiding this comment

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

One query.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants