Direct XCM ExportMessage fees for different bridges to different receiver accounts

[serban300]

Related to #1549 and paritytech/parity-bridges-common#2554

[svyatonik]

LGTM. During review, I realized that I've been sure that we will be distributing fees between different bridges at the sending chain (AH). But obviously I was wrong and BH is the right place to do that.

[svyatonik]

Do not know if there was some design solution behind having Get<Option<AccountId>> in the previous version? Maybe some storage parameter was intended to be used - so fees still may be burnt unless governance has configured chain properly? But in any case - new version looks cleaner to me, just stumbled upon it

[serban300]

I think Get<Option<AccountId>> was needed in order to support the case where we wanted to waive the fees for some locations and burn the ones for the other locations using the XcmFeeToAccount.

But now, that we have a separate component for handling fees, we can use XcmFeeManagerFromComponents to specify the set of waived locations and the handling logic independently. And we can use XcmFeeManagerFromComponents<WaivedLocations, ()> for the scenario above.

[bkontur]

lgtm

[vgeddes]

This is also good for Snowbridge. In our case a portion of the collected export fee will be routed to the sovereign account of the parachain from which the computed origin descends. So we will likely implement a custom XcmFeesToAccount that does this routing.

[KiChjang]

I still think the appropriate place for this is in XcmContext. There may be situations where it's useful for configurable items to know what the ultimate destination for an XCM program is. The problem is whether we should include intermediate destinations -- maybe a BoundedVec<MultiLocation, ConstU32<8>> would do?

[serban300]

@KiChjang

I still think the appropriate place for this is in XcmContext. There may be situations where it's useful for configurable items to know what the ultimate destination for an XCM program is.

I experimented with this a bit, but I'm not sure if I understood correctly. I tried to add a destination field to the XcmContext as Option<MultiLocation> and set it whenever executing an instruction that has a destination. Like TransferReserveAsset, ExportMessage, etc. Is this what you are suggesting ? If so, I don't think this would be the ultimate destination of the XCM program. What about programs that have multiple instructions that call send (not sure, but I guess it's possible) ? For example

Xcm([
    ExportMessage( network: A, destination: A-para-1, xcm: ..),
    ClearOrigin,
    ExportMessage( network: B, destination: B-para-1, xcm: ..),
])

Each instruction could have a different destination, and some instructions might not have any destination at all (would be executed only locally). So considering that this destination is not "ultimate", do you think it would still make sense to add it ?

The problem is whether we should include intermediate destinations -- maybe a BoundedVec<MultiLocation, ConstU32<8>> would do?

I'm not sure if this is possible. From what I understand, the intermediate destinations unfold as we execute the sub-parts of the XCM message on different chains, and the XCM itself doesn't hold a state across multiple chains. Only if we add a separate intermediate_destinations argument to each instruction, or an instruction SetIntermediateDestinations. Or maybe I'm missing something. Could you add more details on this suggestion please ?

[KiChjang]

What does "intermediate destinations unfold as we execute the sub-parts" mean? Why is it not as simple as looking into the xcm parameter in instructions like InitiateTeleport or InitiateReserveWithdraw, and looking for specific arguments in certain instructions, such as the dest parameter of a TransferReserveAsset/DepositReserveAsset/InitiateTeleport instruction?

[serban300]

What does "intermediate destinations unfold as we execute the sub-parts" mean? Why is it not as simple as looking into the xcm parameter in instructions like InitiateTeleport or InitiateReserveWithdraw, and looking for specific arguments in certain instructions, such as the dest parameter of a TransferReserveAsset/DepositReserveAsset/InitiateTeleport instruction?

Sorry, you're right. I don't remember what I was thinking about. Anyway, yes, it would work if we parse the entire message beforehand. But still I think we would have multiple branches, and not an ultimate destination and the route to it.

[gavofyork]

AFAICT, altering FeeHandler trait to read:

pub trait FeeManager {
	/// Determine if a fee which would normally payable should be waived.
	fn is_waived(origin: Option<&MultiLocation>, r: FeeReason) -> bool;

	/// Do something with the fee which has been paid. Doing nothing here silently burns the
	/// fees.
	fn handle_fee(fee: MultiAssets, context: Option<&XcmContext>, r: FeeReason);
}

should be the only API change which is required here. It's easy enough to write type parameterised utility/adapter structs allowing this trait to be composed from components.

The argument which happens to be named dest or destination within one or two XCVM instructions is definitely not a property of the XCVM's context. As an obvious counterpoint, it is conceivable that, in the future, am XCVM instruction exists which has two arguments both of which are some form of "destination".

[gavofyork]

As per my comment.

[KiChjang]

The argument which happens to be named dest or destination within one or two XCVM instructions is definitely not a property of the XCVM's context.

That's not what I'm arguing about. What I am saying is that this PR shouldn't simply modify is_waived to accept an additional FeeReason, but to instead add a destination field for XcmContext, and add it as a parameter to is_waived, since I've seen usage patterns where it can be really beneficial to know which destination the XCM is ultimately destined towards, e.g. when calculating the weight in WeightTrader, thereby justifying the additional destination field in XcmContext.

[serban300]

AFAICT, altering FeeHandler trait to read:

pub trait FeeManager {
	/// Determine if a fee which would normally payable should be waived.
	fn is_waived(origin: Option<&MultiLocation>, r: FeeReason) -> bool;

	/// Do something with the fee which has been paid. Doing nothing here silently burns the
	/// fees.
	fn handle_fee(fee: MultiAssets, context: Option<&XcmContext>, r: FeeReason);
}

should be the only API change which is required here. It's easy enough to write type parameterised utility/adapter structs allowing this trait to be composed from components.

Changed the FeeManager trait as you suggested. But we still need the HandleFee trait in order to chain fee handling strategies. Only moved this logic to a different place. I hope I understood correctly the suggestion.

[gavofyork]

But we still need the HandleFee trait in order to chain fee handling strategies. Only moved this logic to a different place.

Yes, but it's no longer core API but merely a helper trait specific to a utility adapter.

[gavofyork]

Modulo the comments.

[serban300]

@KiChjang sorry, I merged this PR, maybe the changes make it into the next crates release which should be today from what I understand.

Please let's follow-up on the discussion related to adding the destination in the XcmContext in a separate issue/PR if that's ok.