[0.9.40] Weights v2 - Include WeightToFee on XCM Trader

[hbulgarini]

At the moment the XCM Trader configuration at Trappist looks like this:

	type Trader = (
		FixedRateOfFungible<XUsdPerSecond, ()>,
		UsingComponents<WeightToFee, SelfReserve, AccountId, Balances, ToAuthor<Runtime>>,
	);

The UsingComponents trait looks all right and it shouldn't bring any incompatibilities when we migrate to XCM v3/Weights v2.
But i would like to point out the impl FixedRateOfFungible<XUsdPerSecond, ()>, specially the XUsdPerSecond.

Let's take a look at how FixedRateOfFungible is defined:

/// Simple fee calculator that requires payment in a single fungible at a fixed rate.
///
/// The constant `Get` type parameter should be the fungible ID, the amount of it required for one
/// second of weight and the amount required for 1 MB of proof.
pub struct FixedRateOfFungible<T: Get<(AssetId, u128, u128)>, R: TakeRevenue>(
	Weight,
	u128,
	PhantomData<(T, R)>,
);
impl<T: Get<(AssetId, u128, u128)>, R: TakeRevenue> WeightTrader for FixedRateOfFungible<T, R> {
	fn new() -> Self {
		Self(Weight::zero(), 0, PhantomData)
	}

	fn buy_weight(&mut self, weight: Weight, payment: Assets) -> Result<Assets, XcmError> {
		log::trace!(
			target: "xcm::weight",
			"FixedRateOfFungible::buy_weight weight: {:?}, payment: {:?}",
			weight, payment,
		);
		let (id, units_per_second, units_per_mb) = T::get();
		let amount = (units_per_second * (weight.ref_time() as u128) /
			(WEIGHT_REF_TIME_PER_SECOND as u128)) +
			(units_per_mb * (weight.proof_size() as u128) / (WEIGHT_PROOF_SIZE_PER_MB as u128));
		if amount == 0 {
			return Ok(payment)
		}
		let unused =
			payment.checked_sub((id, amount).into()).map_err(|_| XcmError::TooExpensive)?;
		self.0 = self.0.saturating_add(weight);
		self.1 = self.1.saturating_add(amount);
		Ok(unused)
	}

	fn refund_weight(&mut self, weight: Weight) -> Option<MultiAsset> {
		log::trace!(target: "xcm::weight", "FixedRateOfFungible::refund_weight weight: {:?}", weight);
		let (id, units_per_second, units_per_mb) = T::get();
		let weight = weight.min(self.0);
		let amount = (units_per_second * (weight.ref_time() as u128) /
			(WEIGHT_REF_TIME_PER_SECOND as u128)) +
			(units_per_mb * (weight.proof_size() as u128) / (WEIGHT_PROOF_SIZE_PER_MB as u128));
		self.0 -= weight;
		self.1 = self.1.saturating_sub(amount);
		if amount > 0 {
			Some((id, amount).into())
		} else {
			None
		}
	

in the buy_weight function, this is calculation what makes the charges the fee specifically:

let (id, units_per_second, units_per_mb) = T::get();
		let amount = (units_per_second * (weight.ref_time() as u128) /
			(WEIGHT_REF_TIME_PER_SECOND as u128)) +
			(units_per_mb * (weight.proof_size() as u128) / (WEIGHT_PROOF_SIZE_PER_MB as u128));
		if amount == 0 {
			return Ok(payment)
		}

But if we come back as how XUsdPerSecond is defined, we can get the following:

	pub XUsdPerSecond: (xcm::v3::AssetId, u128, u128) = (
		MultiLocation::new(1, X3(Parachain(1000), PalletInstance(50), GeneralIndex(1))).into(),
		default_fee_per_second() * 10
	);

We can see the following definition for default_fee_per_second() :

	pub fn default_fee_per_second() -> u128 {
		let base_weight = Balance::from(ExtrinsicBaseWeight::get().ref_time());
		let base_tx_per_second = (WEIGHT_REF_TIME_PER_SECOND as u128) / base_weight;
		base_tx_per_second * base_tx_fee()
	}

The problem we have with this logic is that ExtrinsicBaseWeight is hardcoded to return 0 for the proof_size:

parameter_types! {
	pub const ExtrinsicBaseWeight: Weight =
		Weight::from_parts(WEIGHT_REF_TIME_PER_NANOS.saturating_mul(113_638), 0);
}

So we could just solve this by simply adding 0 to the XUsdPerSecond and leave the default impl of FixedRateOfFungible to calculate the fee in base of the formula shared above.

Or we could follow Statemine guidelines, where they are using the TakeFirstAssetTrader instead of FixedRateOfFungible.
I will not share all the steps to get into the fee translation formula, but the TakeFirstAssetTrader ends up calculating the amount of the fee with this line:

		let asset_balance: u128 = FeeCharger::charge_weight_in_fungibles(local_asset_id, weight)

Where FeeCharger::charge_weight_in_fungibles(local_asset_id, weight) is the WeightToFee::weight_to_fee from the WeightsToFee implementation:

	fn charge_weight_in_fungibles(
		asset_id: <pallet_assets::Pallet<Runtime, AssetInstance> as Inspect<
			AccountIdOf<Runtime>,
		>>::AssetId,
		weight: Weight,
	) -> Result<
		<pallet_assets::Pallet<Runtime, AssetInstance> as Inspect<AccountIdOf<Runtime>>>::Balance,
		XcmError,
	> {
		let amount = WeightToFee::weight_to_fee(&weight);
		// If the amount gotten is not at least the ED, then make it be the ED of the asset
		// This is to avoid burning assets and decreasing the supply
		let asset_amount = BalanceConverter::to_asset_balance(amount, asset_id)
			.map_err(|_| XcmError::TooExpensive)?;
		Ok(asset_amount)
	}
}

	pub struct WeightToFee;
	impl frame_support::weights::WeightToFee for WeightToFee {
		type Balance = Balance;

		fn weight_to_fee(weight: &Weight) -> Self::Balance {
			let time_poly: FeePolynomial<Balance> = RefTimeToFee::polynomial().into();
			let proof_poly: FeePolynomial<Balance> = ProofSizeToFee::polynomial().into();

			// Take the maximum instead of the sum to charge by the more scarce resource.
			time_poly.eval(weight.ref_time()).max(proof_poly.eval(weight.proof_size()))
		}
	}

This new WeightToFee struct for weights V2 is basically taking the maximum instead of the sum to charge by the more scarce resource, as the comment mentions.

All the development to this new struct on Statemine can be followed here:

paritytech/cumulus#2322
paritytech/substrate#13612
paritytech/cumulus#2326

Also @kalaninja this PR might be interesting for you since you are working with benchmarking atm: paritytech/cumulus#2344

So after all the explanation we have one decision to make:

Should we keep the FixedRateOfFungible and we provide 0 for the proof_size for simplicity sake or should we follow Statemine and rely on the new WeightToFee struct that takes the MAX of both values? My vote goes for the second option.

Then i think that we should adapt the new WeightToFee as it is defined in here:
https://github.com/paritytech/cumulus/blob/master/parachains/runtimes/assets/statemine/src/constants.rs#L57

CC: @stiiifff @valentinfernandez1 @kalaninja

[hbulgarini]

Related: #164

[valentinfernandez1]

I've been looking into the implementation made in paritytech/substrate#13612 and i agree that the way WeightToFee is structured is better but it ends up having the same result at least with the current way WeightsV2 is working.

Even taking a look at benchmarks of write-heavy extrinsics in many substrate pallets, the proof_size seems to always be smaller than the ref_time by an order of 10^4. Egg from the contracts pallet:

fn instantiate_with_code(c: u32, i: u32, s: u32, ) -> Weight {
	// Proof Size summary in bytes:
	//  Measured:  `270`
	//  Estimated: `8659`
	// Minimum execution time: 3_159_921_000 picoseconds.
	Weight::from_parts(594_826_134, 8659)
        ....
}

So comparing both to get the higher value will end up resulting 99.99% of the times in the return of ref_time only adding additional irrelevant calculations.

I agree that FixedRateOfFungible should be removed in favor of TakeFirstAssetTrader but the WaightToFee passed to it should be reconsidered or left with the current implementation, as it achieves the same objective.

Edit: Upon further inspection it looks like it might be possible that the calculated value for the proof becomes higher, but it is dependent on 2 factors. The grade of the polynomial used to calculate it, and the correlation between proof_size and price for the proof.

I will make some extra mathematical analysis next week to see how relevant this is since this information could be very beneficial for the analysis of upgrading the node to WeightsV2.

[stiiifff]

@hbulgarini @valentinfernandez1 We can close this one, right ?